From b2b361926215eadd6bf43ed1d7110b925fc7cae5 Mon Sep 17 00:00:00 2001 From: Tom Harding Date: Wed, 11 Mar 2015 16:48:53 -0700 Subject: [PATCH 1/3] Implement CTransaction::IsEquivalentTo(...) Define CTransaction::IsEquivalentTo(const CTransaction& tx) True if only scriptSigs are different. In other words, true if the two transactions are malleability clones. In other words, true if the two transactions have the same effect on the outside universe. In the wallet, only SyncMetaData for equivalent transactions. --- src/primitives/transaction.cpp | 9 +++++++++ src/primitives/transaction.h | 3 +++ src/wallet/wallet.cpp | 1 + 3 files changed, 13 insertions(+) diff --git a/src/primitives/transaction.cpp b/src/primitives/transaction.cpp index 606dbea79..d864a9b6d 100644 --- 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 index 6cfd93a9a..0c9ebb7b8 100644 --- a/src/primitives/transaction.h +++ b/src/primitives/transaction.h @@ -222,6 +222,9 @@ public: 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 index 2566b2712..92bb972cf 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -417,6 +417,7 @@ void CWallet::SyncMetaData(pair 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 From defd2d55b789163be4a863d0887d5d309ff9cde3 Mon Sep 17 00:00:00 2001 From: Tom Harding Date: Wed, 11 Mar 2015 14:29:06 -0700 Subject: [PATCH 2/3] Better txn_doublespend.py test Remove reliance on accounting "move" ledger entries. Instead, create funding transactions (and deal with fee complexities). Do not rely on broken SyncMetaData. Instead expect double-spend amount to be debited from the default "" account. --- qa/rpc-tests/txn_doublespend.py | 77 ++++++++++++++++++++++----------- 1 file changed, 51 insertions(+), 26 deletions(-) diff --git a/qa/rpc-tests/txn_doublespend.py b/qa/rpc-tests/txn_doublespend.py index fe9168944..955108003 100755 --- 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 import BitcoinTestFramework @@ -32,28 +32,40 @@ class TxnMallTest(BitcoinTestFramework): 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: @@ -65,16 +77,16 @@ class TxnMallTest(BitcoinTestFramework): 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) @@ -85,8 +97,10 @@ class TxnMallTest(BitcoinTestFramework): 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) @@ -104,17 +118,28 @@ class TxnMallTest(BitcoinTestFramework): 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) + # 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 "from" account balance should be just the mutated send: - assert_equal(self.nodes[1].getbalance("from0"), 1210) + # Node1's "from0" account balance should be just the doublespend: + assert_equal(self.nodes[1].getbalance("from0"), 1240) if __name__ == '__main__': TxnMallTest().main() + From 5d34e16d3a1e6ef37d5e6d254063c342b0e5fe39 Mon Sep 17 00:00:00 2001 From: Tom Harding Date: Mon, 23 Mar 2015 20:56:53 -0700 Subject: [PATCH 3/3] Add txn_clone.py test Does what the old txnmall.sh test did. Creates an equivalent malleated clone and tests that SyncMetaData syncs the accounting effects from the original transaction to the confirmed clone. --- qa/pull-tester/rpc-tests.sh | 2 + qa/rpc-tests/txn_clone.py | 167 ++++++++++++++++++++++++++++++++++++ 2 files changed, 169 insertions(+) create mode 100755 qa/rpc-tests/txn_clone.py diff --git a/qa/pull-tester/rpc-tests.sh b/qa/pull-tester/rpc-tests.sh index efeee4553..ffddfa306 100755 --- 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' 'rest.py' 'mempool_spendcoinbase.py' diff --git a/qa/rpc-tests/txn_clone.py b/qa/rpc-tests/txn_clone.py new file mode 100755 index 000000000..19bc34e3f --- /dev/null +++ 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() +