From ff7365e780624a1ef66c12a6d7b61448a3f9294c Mon Sep 17 00:00:00 2001 From: John Newbery Date: Wed, 3 May 2017 11:49:58 -0400 Subject: [PATCH 1/3] [tests] fix flake8 warnings in zapwallettxes.py --- test/functional/zapwallettxes.py | 48 +++++++++++++++++--------------- 1 file changed, 26 insertions(+), 22 deletions(-) diff --git a/test/functional/zapwallettxes.py b/test/functional/zapwallettxes.py index e4d40520e..afe944132 100755 --- a/test/functional/zapwallettxes.py +++ b/test/functional/zapwallettxes.py @@ -13,8 +13,12 @@ available, but that the unconfirmed transaction has been zapped. """ from test_framework.test_framework import BitcoinTestFramework -from test_framework.util import * - +from test_framework.util import (assert_equal, + assert_raises, + bitcoind_processes, + connect_nodes_bi, + JSONRPCException, + ) class ZapWalletTXesTest (BitcoinTestFramework): @@ -25,56 +29,56 @@ class ZapWalletTXesTest (BitcoinTestFramework): def setup_network(self): super().setup_network() - connect_nodes_bi(self.nodes,0,2) + connect_nodes_bi(self.nodes, 0, 2) - def run_test (self): + def run_test(self): self.log.info("Mining blocks...") self.nodes[0].generate(1) self.sync_all() self.nodes[1].generate(101) self.sync_all() - + assert_equal(self.nodes[0].getbalance(), 50) - + txid0 = self.nodes[0].sendtoaddress(self.nodes[2].getnewaddress(), 11) txid1 = self.nodes[0].sendtoaddress(self.nodes[2].getnewaddress(), 10) self.sync_all() self.nodes[0].generate(1) self.sync_all() - + txid2 = self.nodes[0].sendtoaddress(self.nodes[2].getnewaddress(), 11) txid3 = self.nodes[0].sendtoaddress(self.nodes[2].getnewaddress(), 10) - + tx0 = self.nodes[0].gettransaction(txid0) - assert_equal(tx0['txid'], txid0) #tx0 must be available (confirmed) - + assert_equal(tx0['txid'], txid0) # tx0 must be available (confirmed) + tx1 = self.nodes[0].gettransaction(txid1) - assert_equal(tx1['txid'], txid1) #tx1 must be available (confirmed) - + assert_equal(tx1['txid'], txid1) # tx1 must be available (confirmed) + tx2 = self.nodes[0].gettransaction(txid2) - assert_equal(tx2['txid'], txid2) #tx2 must be available (unconfirmed) - + assert_equal(tx2['txid'], txid2) # tx2 must be available (unconfirmed) + tx3 = self.nodes[0].gettransaction(txid3) - assert_equal(tx3['txid'], txid3) #tx3 must be available (unconfirmed) - + assert_equal(tx3['txid'], txid3) # tx3 must be available (unconfirmed) + #restart bitcoind self.stop_node(0) self.nodes[0] = self.start_node(0,self.options.tmpdir) tx3 = self.nodes[0].gettransaction(txid3) - assert_equal(tx3['txid'], txid3) #tx must be available (unconfirmed) - + assert_equal(tx3['txid'], txid3) # tx must be available (unconfirmed) + self.stop_node(0) #restart bitcoind with zapwallettxes self.nodes[0] = self.start_node(0,self.options.tmpdir, ["-zapwallettxes=1"]) - + assert_raises(JSONRPCException, self.nodes[0].gettransaction, [txid3]) - #there must be an exception because the unconfirmed wallettx0 must be gone by now + # there must be an exception because the unconfirmed wallettx0 must be gone by now tx0 = self.nodes[0].gettransaction(txid0) - assert_equal(tx0['txid'], txid0) #tx0 (confirmed) must still be available because it was confirmed + assert_equal(tx0['txid'], txid0) # tx0 (confirmed) must still be available because it was confirmed if __name__ == '__main__': - ZapWalletTXesTest ().main () + ZapWalletTXesTest().main() From e7a2181b49774f2cc29839ebbdc206bcdb715a7f Mon Sep 17 00:00:00 2001 From: John Newbery Date: Wed, 3 May 2017 13:36:13 -0400 Subject: [PATCH 2/3] [wallet] fix zapwallettxes interaction with persistent mempool zapwallettxes previously did not interact well with persistent mempool. zapwallettxes would cause wallet transactions to be zapped, but they would then be reloaded from the mempool on startup. This commit softsets persistmempool to false if zapwallettxes is enabled so transactions are actually zapped. --- src/wallet/wallet.cpp | 5 ++ test/functional/zapwallettxes.py | 82 ++++++++++++++------------------ 2 files changed, 42 insertions(+), 45 deletions(-) diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 07b7f58a6..4366428f5 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -4043,6 +4043,11 @@ bool CWallet::ParameterInteraction() } } + // -zapwallettx implies dropping the mempool on startup + if (GetBoolArg("-zapwallettxes", false) && SoftSetBoolArg("-persistmempool", false)) { + LogPrintf("%s: parameter interaction: -zapwallettxes= -> setting -persistmempool=0\n", __func__); + } + // -zapwallettx implies a rescan if (GetBoolArg("-zapwallettxes", false)) { if (is_multiwallet) { diff --git a/test/functional/zapwallettxes.py b/test/functional/zapwallettxes.py index afe944132..af867d7a5 100755 --- a/test/functional/zapwallettxes.py +++ b/test/functional/zapwallettxes.py @@ -4,20 +4,19 @@ # file COPYING or http://www.opensource.org/licenses/mit-license.php. """Test the zapwallettxes functionality. -- start three bitcoind nodes -- create four transactions on node 0 - two are confirmed and two are - unconfirmed. -- restart node 1 and verify that both the confirmed and the unconfirmed +- start two bitcoind nodes +- create two transactions on node 0 - one is confirmed and one is unconfirmed. +- restart node 0 and verify that both the confirmed and the unconfirmed transactions are still available. -- restart node 0 and verify that the confirmed transactions are still - available, but that the unconfirmed transaction has been zapped. +- restart node 0 with zapwallettxes and persistmempool, and verify that both + the confirmed and the unconfirmed transactions are still available. +- restart node 0 with just zapwallettxed and verify that the confirmed + transactions are still available, but that the unconfirmed transaction has + been zapped. """ from test_framework.test_framework import BitcoinTestFramework from test_framework.util import (assert_equal, - assert_raises, - bitcoind_processes, - connect_nodes_bi, - JSONRPCException, + assert_raises_jsonrpc, ) class ZapWalletTXesTest (BitcoinTestFramework): @@ -25,60 +24,53 @@ class ZapWalletTXesTest (BitcoinTestFramework): def __init__(self): super().__init__() self.setup_clean_chain = True - self.num_nodes = 3 - - def setup_network(self): - super().setup_network() - connect_nodes_bi(self.nodes, 0, 2) + self.num_nodes = 2 def run_test(self): self.log.info("Mining blocks...") self.nodes[0].generate(1) self.sync_all() - self.nodes[1].generate(101) + self.nodes[1].generate(100) self.sync_all() - assert_equal(self.nodes[0].getbalance(), 50) + # This transaction will be confirmed + txid1 = self.nodes[0].sendtoaddress(self.nodes[1].getnewaddress(), 10) - txid0 = self.nodes[0].sendtoaddress(self.nodes[2].getnewaddress(), 11) - txid1 = self.nodes[0].sendtoaddress(self.nodes[2].getnewaddress(), 10) - self.sync_all() self.nodes[0].generate(1) self.sync_all() - txid2 = self.nodes[0].sendtoaddress(self.nodes[2].getnewaddress(), 11) - txid3 = self.nodes[0].sendtoaddress(self.nodes[2].getnewaddress(), 10) + # This transaction will not be confirmed + txid2 = self.nodes[0].sendtoaddress(self.nodes[1].getnewaddress(), 20) - tx0 = self.nodes[0].gettransaction(txid0) - assert_equal(tx0['txid'], txid0) # tx0 must be available (confirmed) + # Confirmed and unconfirmed transactions are now in the wallet. + assert_equal(self.nodes[0].gettransaction(txid1)['txid'], txid1) + assert_equal(self.nodes[0].gettransaction(txid2)['txid'], txid2) - tx1 = self.nodes[0].gettransaction(txid1) - assert_equal(tx1['txid'], txid1) # tx1 must be available (confirmed) - - tx2 = self.nodes[0].gettransaction(txid2) - assert_equal(tx2['txid'], txid2) # tx2 must be available (unconfirmed) - - tx3 = self.nodes[0].gettransaction(txid3) - assert_equal(tx3['txid'], txid3) # tx3 must be available (unconfirmed) - - #restart bitcoind + # Stop-start node0. Both confirmed and unconfirmed transactions remain in the wallet. self.stop_node(0) - self.nodes[0] = self.start_node(0,self.options.tmpdir) - - tx3 = self.nodes[0].gettransaction(txid3) - assert_equal(tx3['txid'], txid3) # tx must be available (unconfirmed) + self.nodes[0] = self.start_node(0, self.options.tmpdir) + assert_equal(self.nodes[0].gettransaction(txid1)['txid'], txid1) + assert_equal(self.nodes[0].gettransaction(txid2)['txid'], txid2) + + # Stop node0 and restart with zapwallettxes and persistmempool. The unconfirmed + # transaction is zapped from the wallet, but is re-added when the mempool is reloaded. self.stop_node(0) - - #restart bitcoind with zapwallettxes - self.nodes[0] = self.start_node(0,self.options.tmpdir, ["-zapwallettxes=1"]) + self.nodes[0] = self.start_node(0, self.options.tmpdir, ["-persistmempool=1", "-zapwallettxes=2"]) - assert_raises(JSONRPCException, self.nodes[0].gettransaction, [txid3]) - # there must be an exception because the unconfirmed wallettx0 must be gone by now + assert_equal(self.nodes[0].gettransaction(txid1)['txid'], txid1) + assert_equal(self.nodes[0].gettransaction(txid2)['txid'], txid2) - tx0 = self.nodes[0].gettransaction(txid0) - assert_equal(tx0['txid'], txid0) # tx0 (confirmed) must still be available because it was confirmed + # Stop node0 and restart with zapwallettxes, but not persistmempool. + # The unconfirmed transaction is zapped and is no longer in the wallet. + self.stop_node(0) + self.nodes[0] = self.start_node(0, self.options.tmpdir, ["-zapwallettxes=2"]) + # tx1 is still be available because it was confirmed + assert_equal(self.nodes[0].gettransaction(txid1)['txid'], txid1) + + # This will raise an exception because the unconfirmed transaction has been zapped + assert_raises_jsonrpc(-5, 'Invalid or non-wallet transaction id', self.nodes[0].gettransaction, txid2) if __name__ == '__main__': ZapWalletTXesTest().main() From 4c3b538c61532dc68d79bbe34729759a13b73f0c Mon Sep 17 00:00:00 2001 From: John Newbery Date: Fri, 14 Jul 2017 18:17:27 -0400 Subject: [PATCH 3/3] [logs] fix zapwallettxes startup logs --- src/wallet/wallet.cpp | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 4366428f5..7aa2e3174 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -4043,18 +4043,19 @@ bool CWallet::ParameterInteraction() } } - // -zapwallettx implies dropping the mempool on startup - if (GetBoolArg("-zapwallettxes", false) && SoftSetBoolArg("-persistmempool", false)) { - LogPrintf("%s: parameter interaction: -zapwallettxes= -> setting -persistmempool=0\n", __func__); + int zapwallettxes = GetArg("-zapwallettxes", 0); + // -zapwallettxes implies dropping the mempool on startup + if (zapwallettxes != 0 && SoftSetBoolArg("-persistmempool", false)) { + LogPrintf("%s: parameter interaction: -zapwallettxes=%s -> setting -persistmempool=0\n", __func__, zapwallettxes); } - // -zapwallettx implies a rescan - if (GetBoolArg("-zapwallettxes", false)) { + // -zapwallettxes implies a rescan + if (zapwallettxes != 0) { if (is_multiwallet) { return InitError(strprintf("%s is only allowed with a single wallet file", "-zapwallettxes")); } if (SoftSetBoolArg("-rescan", true)) { - LogPrintf("%s: parameter interaction: -zapwallettxes= -> setting -rescan=1\n", __func__); + LogPrintf("%s: parameter interaction: -zapwallettxes=%s -> setting -rescan=1\n", __func__, zapwallettxes); } }