Merge pull request bitcoin#7229

fa33d97 [walletdb] Add missing LOCK() in Recover() for dummyWallet (MarcoFalke)
fa14d99 [qa] check if wallet or blochchain maintenance changes the balance (MarcoFalke)
fa0765d [qa] Cleanup wallet.py test (MarcoFalke)

Author: laanwj

qa/rpc-tests/wallet.py

@@ -3,21 +3,6 @@
 # Distributed under the MIT software license, see the accompanying
 # file COPYING or http://www.opensource.org/licenses/mit-license.php.
 
-#
-# Exercise the wallet.  Ported from wallet.sh.
-# Does the following:
-#   a) creates 3 nodes, with an empty chain (no blocks).
-#   b) node0 mines a block
-#   c) node1 mines 101 blocks, so now nodes 0 and 1 have 50btc, node2 has none.
-#   d) node0 sends 21 btc to node2, in two transactions (11 btc, then 10 btc).
-#   e) node0 mines a block, collects the fee on the second transaction
-#   f) node1 mines 100 blocks, to mature node0's just-mined block
-#   g) check that node0 has 100-21, node2 has 21
-#   h) node0 should now have 2 unspent outputs;  send these to node2 via raw tx broadcast by node1
-#   i) have node1 mine a block
-#   j) check balances - node0 should have 0, node2 should have 100
-#   k) test ResendWalletTransactions - create transactions, startup fourth node, make sure it syncs
-#
 
 from test_framework.test_framework import BitcoinTestFramework
 from test_framework.util import *
@@ -190,7 +175,7 @@ def run_test (self):
         for uTx in unspentTxs:
             if uTx['txid'] == zeroValueTxid:
                 found = True
-                assert_equal(uTx['amount'], Decimal('0.00000000'));
+                assert_equal(uTx['amount'], Decimal('0'))
         assert(found)
 
         #do some -walletbroadcast tests
@@ -202,21 +187,22 @@ def run_test (self):
         connect_nodes_bi(self.nodes,0,2)
         self.sync_all()
 
-        txIdNotBroadcasted  = self.nodes[0].sendtoaddress(self.nodes[2].getnewaddress(), 2);
+        txIdNotBroadcasted  = self.nodes[0].sendtoaddress(self.nodes[2].getnewaddress(), 2)
         txObjNotBroadcasted = self.nodes[0].gettransaction(txIdNotBroadcasted)
         self.nodes[1].generate(1) #mine a block, tx should not be in there
         self.sync_all()
-        assert_equal(self.nodes[2].getbalance(), node_2_bal); #should not be changed because tx was not broadcasted
+        assert_equal(self.nodes[2].getbalance(), node_2_bal) #should not be changed because tx was not broadcasted
 
         #now broadcast from another node, mine a block, sync, and check the balance
         self.nodes[1].sendrawtransaction(txObjNotBroadcasted['hex'])
         self.nodes[1].generate(1)
         self.sync_all()
+        node_2_bal += 2
         txObjNotBroadcasted = self.nodes[0].gettransaction(txIdNotBroadcasted)
-        assert_equal(self.nodes[2].getbalance(), node_2_bal + Decimal('2')); #should not be
+        assert_equal(self.nodes[2].getbalance(), node_2_bal)
 
         #create another tx
-        txIdNotBroadcasted  = self.nodes[0].sendtoaddress(self.nodes[2].getnewaddress(), 2);
+        txIdNotBroadcasted  = self.nodes[0].sendtoaddress(self.nodes[2].getnewaddress(), 2)
 
         #restart the nodes with -walletbroadcast=1
         stop_nodes(self.nodes)
@@ -229,23 +215,24 @@ def run_test (self):
 
         self.nodes[0].generate(1)
         sync_blocks(self.nodes)
+        node_2_bal += 2
 
         #tx should be added to balance because after restarting the nodes tx should be broadcastet
-        assert_equal(self.nodes[2].getbalance(), node_2_bal + Decimal('4')); #should not be
+        assert_equal(self.nodes[2].getbalance(), node_2_bal)
 
         #send a tx with value in a string (PR#6380 +)
         txId  = self.nodes[0].sendtoaddress(self.nodes[2].getnewaddress(), "2")
         txObj = self.nodes[0].gettransaction(txId)
-        assert_equal(txObj['amount'], Decimal('-2.00000000'))
+        assert_equal(txObj['amount'], Decimal('-2'))
 
         txId  = self.nodes[0].sendtoaddress(self.nodes[2].getnewaddress(), "0.0001")
         txObj = self.nodes[0].gettransaction(txId)
-        assert_equal(txObj['amount'], Decimal('-0.00010000'))
+        assert_equal(txObj['amount'], Decimal('-0.0001'))
 
         #check if JSON parser can handle scientific notation in strings
         txId  = self.nodes[0].sendtoaddress(self.nodes[2].getnewaddress(), "1e-4")
         txObj = self.nodes[0].gettransaction(txId)
-        assert_equal(txObj['amount'], Decimal('-0.00010000'))
+        assert_equal(txObj['amount'], Decimal('-0.0001'))
 
         #this should fail
         errorString = ""
@@ -254,15 +241,38 @@ def run_test (self):
         except JSONRPCException,e:
             errorString = e.error['message']
 
-        assert_equal("Invalid amount" in errorString, True);
+        assert_equal("Invalid amount" in errorString, True)
 
         errorString = ""
         try:
             self.nodes[0].generate("2") #use a string to as block amount parameter must fail because it's not interpreted as amount
         except JSONRPCException,e:
             errorString = e.error['message']
 
-        assert_equal("not an integer" in errorString, True);
+        assert_equal("not an integer" in errorString, True)
+
+        #check if wallet or blochchain maintenance changes the balance
+        self.sync_all()
+        self.nodes[0].generate(1)
+        self.sync_all()
+        balance_nodes = [self.nodes[i].getbalance() for i in range(3)]
+
+        maintenance = [
+            '-rescan',
+            '-reindex',
+            '-zapwallettxes=1',
+            '-zapwallettxes=2',
+            '-salvagewallet',
+        ]
+        for m in maintenance:
+            stop_nodes(self.nodes)
+            wait_bitcoinds()
+            self.nodes = start_nodes(3, self.options.tmpdir, [[m]] * 3)
+            connect_nodes_bi(self.nodes,0,1)
+            connect_nodes_bi(self.nodes,1,2)
+            connect_nodes_bi(self.nodes,0,2)
+            self.sync_all()
+            assert_equal(balance_nodes, [self.nodes[i].getbalance() for i in range(3)])
 
 
 if __name__ == '__main__':

src/wallet/walletdb.cpp

@@ -966,8 +966,13 @@ bool CWalletDB::Recover(CDBEnv& dbenv, const std::string& filename, bool fOnlyKe
             CDataStream ssKey(row.first, SER_DISK, CLIENT_VERSION);
             CDataStream ssValue(row.second, SER_DISK, CLIENT_VERSION);
             string strType, strErr;
-            bool fReadOK = ReadKeyValue(&dummyWallet, ssKey, ssValue,
+            bool fReadOK;
+            {
+                // Required in LoadKeyMetadata():
+                LOCK(dummyWallet.cs_wallet);
+                fReadOK = ReadKeyValue(&dummyWallet, ssKey, ssValue,
                                         wss, strType, strErr);
+            }
             if (!IsKeyType(strType))
                 continue;
             if (!fReadOK)