Merge pull request #5881

laanwj · laanwj · commit 3203a0832adf · 2015-07-02T19:59:50.000+02:00
5d34e16 Add txn_clone.py test (Tom Harding) defd2d5 Better txn_doublespend.py test (Tom Harding) b2b3619 Implement CTransaction::IsEquivalentTo(...) (Tom Harding)
diff --git a/qa/pull-tester/rpc-tests.sh b/qa/pull-tester/rpc-tests.sh
@@ -21,6 +21,8 @@ testScripts=(
     'mempool_resurrect_test.py'
     'txn_doublespend.py'
     'txn_doublespend.py --mineblock'
+    'txn_clone.py'
+    'txn_clone.py --mineblock'
     'getchaintips.py'
     'rawtransactions.py'
     'rest.py'
diff --git a/qa/rpc-tests/txn_clone.py b/qa/rpc-tests/txn_clone.py
@@ -0,0 +1,167 @@
+#!/usr/bin/env python2
+# Copyright (c) 2014 The Bitcoin Core developers
+# Distributed under the MIT software license, see the accompanying
+# file COPYING or http://www.opensource.org/licenses/mit-license.php.
+
+#
+# Test proper accounting with an equivalent malleability clone
+#
+
+from test_framework import BitcoinTestFramework
+from bitcoinrpc.authproxy import AuthServiceProxy, JSONRPCException
+from decimal import Decimal
+from util import *
+import os
+import shutil
+
+class TxnMallTest(BitcoinTestFramework):
+
+    def add_options(self, parser):
+        parser.add_option("--mineblock", dest="mine_block", default=False, action="store_true",
+                          help="Test double-spend of 1-confirmed transaction")
+
+    def setup_network(self):
+        # Start with split network:
+        return super(TxnMallTest, self).setup_network(True)
+
+    def run_test(self):
+        # All nodes should start with 1,250 BTC:
+        starting_balance = 1250
+        for i in range(4):
+            assert_equal(self.nodes[i].getbalance(), starting_balance)
+            self.nodes[i].getnewaddress("")  # bug workaround, coins generated assigned to first getnewaddress!
+
+        # Assign coins to foo and bar accounts:
+        self.nodes[0].settxfee(.001)
+
+        node0_address_foo = self.nodes[0].getnewaddress("foo")
+        fund_foo_txid = self.nodes[0].sendfrom("", node0_address_foo, 1219)
+        fund_foo_tx = self.nodes[0].gettransaction(fund_foo_txid)
+
+        node0_address_bar = self.nodes[0].getnewaddress("bar")
+        fund_bar_txid = self.nodes[0].sendfrom("", node0_address_bar, 29)
+        fund_bar_tx = self.nodes[0].gettransaction(fund_bar_txid)
+
+        assert_equal(self.nodes[0].getbalance(""),
+                     starting_balance - 1219 - 29 + fund_foo_tx["fee"] + fund_bar_tx["fee"])
+
+        # Coins are sent to node1_address
+        node1_address = self.nodes[1].getnewaddress("from0")
+
+        # Send tx1, and another transaction tx2 that won't be cloned 
+        txid1 = self.nodes[0].sendfrom("foo", node1_address, 40, 0)
+        txid2 = self.nodes[0].sendfrom("bar", node1_address, 20, 0)
+
+        # Construct a clone of tx1, to be malleated 
+        rawtx1 = self.nodes[0].getrawtransaction(txid1,1)
+        clone_inputs = [{"txid":rawtx1["vin"][0]["txid"],"vout":rawtx1["vin"][0]["vout"]}]
+        clone_outputs = {rawtx1["vout"][0]["scriptPubKey"]["addresses"][0]:rawtx1["vout"][0]["value"],
+                         rawtx1["vout"][1]["scriptPubKey"]["addresses"][0]:rawtx1["vout"][1]["value"]}
+        clone_raw = self.nodes[0].createrawtransaction(clone_inputs, clone_outputs)
+
+        # 3 hex manipulations on the clone are required
+
+        # manipulation 1. sequence is at version+#inputs+input+sigstub
+        posseq = 2*(4+1+36+1)
+        seqbe = '%08x' % rawtx1["vin"][0]["sequence"]
+        clone_raw = clone_raw[:posseq] + seqbe[6:8] + seqbe[4:6] + seqbe[2:4] + seqbe[0:2] + clone_raw[posseq + 8:]
+
+        # manipulation 2. createrawtransaction randomizes the order of its outputs, so swap them if necessary.
+        # output 0 is at version+#inputs+input+sigstub+sequence+#outputs
+        # 40 BTC serialized is 00286bee00000000
+        pos0 = 2*(4+1+36+1+4+1)
+        hex40 = "00286bee00000000"
+        output_len = 16 + 2 + 2 * int("0x" + clone_raw[pos0 + 16 : pos0 + 16 + 2], 0)
+        if (rawtx1["vout"][0]["value"] == 40 and clone_raw[pos0 : pos0 + 16] != hex40 or
+            rawtx1["vout"][0]["value"] != 40 and clone_raw[pos0 : pos0 + 16] == hex40):
+            output0 = clone_raw[pos0 : pos0 + output_len]
+            output1 = clone_raw[pos0 + output_len : pos0 + 2 * output_len]
+            clone_raw = clone_raw[:pos0] + output1 + output0 + clone_raw[pos0 + 2 * output_len:]
+
+        # manipulation 3. locktime is after outputs
+        poslt = pos0 + 2 * output_len
+        ltbe = '%08x' % rawtx1["locktime"]
+        clone_raw = clone_raw[:poslt] + ltbe[6:8] + ltbe[4:6] + ltbe[2:4] + ltbe[0:2] + clone_raw[poslt + 8:]
+
+        # Use a different signature hash type to sign.  This creates an equivalent but malleated clone.
+        # Don't send the clone anywhere yet
+        tx1_clone = self.nodes[0].signrawtransaction(clone_raw, None, None, "ALL|ANYONECANPAY")
+        assert_equal(tx1_clone["complete"], True)
+
+        # Have node0 mine a block, if requested:
+        if (self.options.mine_block):
+            self.nodes[0].generate(1)
+            sync_blocks(self.nodes[0:2])
+
+        tx1 = self.nodes[0].gettransaction(txid1)
+        tx2 = self.nodes[0].gettransaction(txid2)
+
+        # Node0's balance should be starting balance, plus 50BTC for another
+        # matured block, minus tx1 and tx2 amounts, and minus transaction fees:
+        expected = starting_balance + fund_foo_tx["fee"] + fund_bar_tx["fee"]
+        if self.options.mine_block: expected += 50
+        expected += tx1["amount"] + tx1["fee"]
+        expected += tx2["amount"] + tx2["fee"]
+        assert_equal(self.nodes[0].getbalance(), expected)
+
+        # foo and bar accounts should be debited:
+        assert_equal(self.nodes[0].getbalance("foo", 0), 1219 + tx1["amount"] + tx1["fee"])
+        assert_equal(self.nodes[0].getbalance("bar", 0), 29 + tx2["amount"] + tx2["fee"])
+
+        if self.options.mine_block:
+            assert_equal(tx1["confirmations"], 1)
+            assert_equal(tx2["confirmations"], 1)
+            # Node1's "from0" balance should be both transaction amounts:
+            assert_equal(self.nodes[1].getbalance("from0"), -(tx1["amount"] + tx2["amount"]))
+        else:
+            assert_equal(tx1["confirmations"], 0)
+            assert_equal(tx2["confirmations"], 0)
+
+        # Send clone and its parent to miner
+        self.nodes[2].sendrawtransaction(fund_foo_tx["hex"])
+        txid1_clone = self.nodes[2].sendrawtransaction(tx1_clone["hex"])
+        # ... mine a block...
+        self.nodes[2].generate(1)
+
+        # Reconnect the split network, and sync chain:
+        connect_nodes(self.nodes[1], 2)
+        self.nodes[2].generate(1)  # Mine another block to make sure we sync
+        sync_blocks(self.nodes)
+
+        # Re-fetch transaction info:
+        tx1 = self.nodes[0].gettransaction(txid1)
+        tx1_clone = self.nodes[0].gettransaction(txid1_clone)
+        tx2 = self.nodes[0].gettransaction(txid2)
+        
+        # Verify expected confirmations
+        assert_equal(tx1["confirmations"], -1)
+        assert_equal(tx1_clone["confirmations"], 2)
+        assert_equal(tx2["confirmations"], 0)
+
+        # Check node0's total balance; should be same as before the clone, + 100 BTC for 2 matured,
+        # less possible orphaned matured subsidy
+        expected += 100
+        if (self.options.mine_block): 
+            expected -= 50
+        assert_equal(self.nodes[0].getbalance(), expected)
+        assert_equal(self.nodes[0].getbalance("*", 0), expected)
+
+        # Check node0's individual account balances.
+        # "foo" should have been debited by the equivalent clone of tx1
+        assert_equal(self.nodes[0].getbalance("foo"), 1219 + tx1["amount"] + tx1["fee"])
+        # "bar" should have been debited by (possibly unconfirmed) tx2
+        assert_equal(self.nodes[0].getbalance("bar", 0), 29 + tx2["amount"] + tx2["fee"])
+        # "" should have starting balance, less funding txes, plus subsidies
+        assert_equal(self.nodes[0].getbalance("", 0), starting_balance
+                                                                - 1219
+                                                                + fund_foo_tx["fee"]
+                                                                -   29
+                                                                + fund_bar_tx["fee"]
+                                                                +  100)
+
+        # Node1's "from0" account balance
+        assert_equal(self.nodes[1].getbalance("from0", 0), -(tx1["amount"] + tx2["amount"]))
+
+if __name__ == '__main__':
+    TxnMallTest().main()
+
diff --git a/qa/rpc-tests/txn_doublespend.py b/qa/rpc-tests/txn_doublespend.py
@@ -4,7 +4,7 @@
 # file COPYING or http://www.opensource.org/licenses/mit-license.php.
 
 #
-# Test proper accounting with malleable transactions
+# Test proper accounting with a double-spend conflict
 #
 
 from test_framework.test_framework import BitcoinTestFramework
@@ -31,28 +31,40 @@ def run_test(self):
             self.nodes[i].getnewaddress("")  # bug workaround, coins generated assigned to first getnewaddress!
         
         # Assign coins to foo and bar accounts:
-        self.nodes[0].move("", "foo", 1220)
-        self.nodes[0].move("", "bar", 30)
-        assert_equal(self.nodes[0].getbalance(""), 0)
+        node0_address_foo = self.nodes[0].getnewaddress("foo")
+        fund_foo_txid = self.nodes[0].sendfrom("", node0_address_foo, 1219)
+        fund_foo_tx = self.nodes[0].gettransaction(fund_foo_txid)
+
+        node0_address_bar = self.nodes[0].getnewaddress("bar")
+        fund_bar_txid = self.nodes[0].sendfrom("", node0_address_bar, 29)
+        fund_bar_tx = self.nodes[0].gettransaction(fund_bar_txid)
+
+        assert_equal(self.nodes[0].getbalance(""),
+                     starting_balance - 1219 - 29 + fund_foo_tx["fee"] + fund_bar_tx["fee"])
 
         # Coins are sent to node1_address
         node1_address = self.nodes[1].getnewaddress("from0")
 
-        # First: use raw transaction API to send 1210 BTC to node1_address,
+        # First: use raw transaction API to send 1240 BTC to node1_address,
         # but don't broadcast:
-        (total_in, inputs) = gather_inputs(self.nodes[0], 1210)
-        change_address = self.nodes[0].getnewaddress("foo")
+        doublespend_fee = Decimal('-.02')
+        rawtx_input_0 = {}
+        rawtx_input_0["txid"] = fund_foo_txid
+        rawtx_input_0["vout"] = find_output(self.nodes[0], fund_foo_txid, 1219)
+        rawtx_input_1 = {}
+        rawtx_input_1["txid"] = fund_bar_txid
+        rawtx_input_1["vout"] = find_output(self.nodes[0], fund_bar_txid, 29)
+        inputs = [rawtx_input_0, rawtx_input_1]
+        change_address = self.nodes[0].getnewaddress()
         outputs = {}
-        outputs[change_address] = 40
-        outputs[node1_address] = 1210
+        outputs[node1_address] = 1240
+        outputs[change_address] = 1248 - 1240 + doublespend_fee
         rawtx = self.nodes[0].createrawtransaction(inputs, outputs)
         doublespend = self.nodes[0].signrawtransaction(rawtx)
         assert_equal(doublespend["complete"], True)
 
-        # Create two transaction from node[0] to node[1]; the
-        # second must spend change from the first because the first
-        # spends all mature inputs:
-        txid1 = self.nodes[0].sendfrom("foo", node1_address, 1210, 0)
+        # Create two spends using 1 50 BTC coin each
+        txid1 = self.nodes[0].sendfrom("foo", node1_address, 40, 0)
         txid2 = self.nodes[0].sendfrom("bar", node1_address, 20, 0)
         
         # Have node0 mine a block:
@@ -64,16 +76,16 @@ def run_test(self):
         tx2 = self.nodes[0].gettransaction(txid2)
 
         # Node0's balance should be starting balance, plus 50BTC for another
-        # matured block, minus 1210, minus 20, and minus transaction fees:
-        expected = starting_balance
+        # matured block, minus 40, minus 20, and minus transaction fees:
+        expected = starting_balance + fund_foo_tx["fee"] + fund_bar_tx["fee"]
         if self.options.mine_block: expected += 50
         expected += tx1["amount"] + tx1["fee"]
         expected += tx2["amount"] + tx2["fee"]
         assert_equal(self.nodes[0].getbalance(), expected)
 
         # foo and bar accounts should be debited:
-        assert_equal(self.nodes[0].getbalance("foo"), 1220+tx1["amount"]+tx1["fee"])
-        assert_equal(self.nodes[0].getbalance("bar"), 30+tx2["amount"]+tx2["fee"])
+        assert_equal(self.nodes[0].getbalance("foo", 0), 1219+tx1["amount"]+tx1["fee"])
+        assert_equal(self.nodes[0].getbalance("bar", 0), 29+tx2["amount"]+tx2["fee"])
 
         if self.options.mine_block:
             assert_equal(tx1["confirmations"], 1)
@@ -84,8 +96,10 @@ def run_test(self):
             assert_equal(tx1["confirmations"], 0)
             assert_equal(tx2["confirmations"], 0)
         
-        # Now give doublespend to miner:
-        mutated_txid = self.nodes[2].sendrawtransaction(doublespend["hex"])
+        # Now give doublespend and its parents to miner:
+        self.nodes[2].sendrawtransaction(fund_foo_tx["hex"])
+        self.nodes[2].sendrawtransaction(fund_bar_tx["hex"])
+        self.nodes[2].sendrawtransaction(doublespend["hex"])
         # ... mine a block...
         self.nodes[2].generate(1)
 
@@ -103,17 +117,28 @@ def run_test(self):
         assert_equal(tx2["confirmations"], -1)
 
         # Node0's total balance should be starting balance, plus 100BTC for 
-        # two more matured blocks, minus 1210 for the double-spend:
-        expected = starting_balance + 100 - 1210
+        # two more matured blocks, minus 1240 for the double-spend, plus fees (which are
+        # negative):
+        expected = starting_balance + 100 - 1240 + fund_foo_tx["fee"] + fund_bar_tx["fee"] + doublespend_fee
         assert_equal(self.nodes[0].getbalance(), expected)
         assert_equal(self.nodes[0].getbalance("*"), expected)
 
-        # foo account should be debited, but bar account should not:
-        assert_equal(self.nodes[0].getbalance("foo"), 1220-1210)
-        assert_equal(self.nodes[0].getbalance("bar"), 30)
-
-        # Node1's "from" account balance should be just the mutated send:
-        assert_equal(self.nodes[1].getbalance("from0"), 1210)
+        # Final "" balance is starting_balance - amount moved to accounts - doublespend + subsidies +
+        # fees (which are negative)
+        assert_equal(self.nodes[0].getbalance("foo"), 1219)
+        assert_equal(self.nodes[0].getbalance("bar"), 29)
+        assert_equal(self.nodes[0].getbalance(""), starting_balance
+                                                              -1219
+                                                              -  29
+                                                              -1240
+                                                              + 100
+                                                              + fund_foo_tx["fee"]
+                                                              + fund_bar_tx["fee"]
+                                                              + doublespend_fee)
+
+        # Node1's "from0" account balance should be just the doublespend:
+        assert_equal(self.nodes[1].getbalance("from0"), 1240)
 
 if __name__ == '__main__':
     TxnMallTest().main()
+
diff --git a/src/primitives/transaction.cpp b/src/primitives/transaction.cpp
@@ -87,6 +87,15 @@ CTransaction& CTransaction::operator=(const CTransaction &tx) {
     return *this;
 }
 
+bool CTransaction::IsEquivalentTo(const CTransaction& tx) const
+{
+	CMutableTransaction tx1 = *this;
+	CMutableTransaction tx2 = tx;
+	for (unsigned int i = 0; i < tx1.vin.size(); i++) tx1.vin[i].scriptSig = CScript();
+	for (unsigned int i = 0; i < tx2.vin.size(); i++) tx2.vin[i].scriptSig = CScript();
+	return CTransaction(tx1) == CTransaction(tx2);
+}
+
 CAmount CTransaction::GetValueOut() const
 {
     CAmount nValueOut = 0;
diff --git a/src/primitives/transaction.h b/src/primitives/transaction.h
@@ -222,6 +222,9 @@ class CTransaction
         return hash;
     }
 
+    // True if only scriptSigs are different
+    bool IsEquivalentTo(const CTransaction& tx) const;
+
     // Return sum of txouts.
     CAmount GetValueOut() const;
     // GetValueIn() is a method on CCoinsViewCache, because
diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp
@@ -419,6 +419,7 @@ void CWallet::SyncMetaData(pair<TxSpends::iterator, TxSpends::iterator> range)
         const uint256& hash = it->second;
         CWalletTx* copyTo = &mapWallet[hash];
         if (copyFrom == copyTo) continue;
+        if (!copyFrom->IsEquivalentTo(*copyTo)) continue;
         copyTo->mapValue = copyFrom->mapValue;
         copyTo->vOrderForm = copyFrom->vOrderForm;
         // fTimeReceivedIsTxTime not copied on purpose

Original file line number	Diff line number	Diff line change
`@@ -222,6 +222,9 @@ class CTransaction`
`222`	`222`	`return hash;`
`223`	`223`	`}`
`224`	`224`
	`225`	`+ // True if only scriptSigs are different`
	`226`	`+ bool IsEquivalentTo(const CTransaction& tx) const;`
	`227`	`+`
`225`	`228`	`// Return sum of txouts.`
`226`	`229`	`CAmount GetValueOut() const;`
`227`	`230`	`// GetValueIn() is a method on CCoinsViewCache, because`