sapling_wallet_tests: locking order refactor, solving inconsistent or…

…ders for cs_main and cs_wallet.
PIVX-Project · Mar 10, 2021 · e0a0d2d · e0a0d2d
1 parent c69e7e8
commit e0a0d2d
Show file tree

Hide file tree

Showing 3 changed files with 42 additions and 30 deletions.
diff --git a/src/test/librust/sapling_wallet_tests.cpp b/src/test/librust/sapling_wallet_tests.cpp
@@ -614,8 +614,10 @@ BOOST_AUTO_TEST_CASE(CachedWitnessesEmptyChain) {
     auto consensusParams = RegtestActivateSapling();
 
     CWallet& wallet = *pwalletMain;
-    LOCK(wallet.cs_wallet);
-    setupWallet(wallet);
+    {
+        LOCK(wallet.cs_wallet);
+        setupWallet(wallet);
+    }
 
     auto sk = GetTestMasterSaplingSpendingKey();
     CWalletTx wtx = GetValidSaplingReceive(Params().GetConsensus(), wallet, sk, 10, true);
@@ -653,18 +655,19 @@ BOOST_AUTO_TEST_CASE(CachedWitnessesEmptyChain) {
 BOOST_AUTO_TEST_CASE(CachedWitnessesChainTip) {
     auto consensusParams = RegtestActivateSapling();
 
+    libzcash::SaplingExtendedSpendingKey sk = GetTestMasterSaplingSpendingKey();
     CWallet& wallet = *pwalletMain;
-    LOCK(wallet.cs_wallet);
-    setupWallet(wallet);
+    {
+        LOCK(wallet.cs_wallet);
+        setupWallet(wallet);
+        BOOST_CHECK(wallet.AddSaplingZKey(sk));
+        BOOST_CHECK(wallet.HaveSaplingSpendingKey(sk.ToXFVK()));
+    }
 
     uint256 anchors1;
     CBlock block1;
     SaplingMerkleTree saplingTree;
 
-    libzcash::SaplingExtendedSpendingKey sk = GetTestMasterSaplingSpendingKey();
-    BOOST_CHECK(wallet.AddSaplingZKey(sk));
-    BOOST_CHECK(wallet.HaveSaplingSpendingKey(sk.ToXFVK()));
-
     {
         // First block (case tested in _empty_chain)
         CBlockIndex index1(block1);
@@ -731,15 +734,15 @@ BOOST_AUTO_TEST_CASE(CachedWitnessesChainTip) {
 BOOST_AUTO_TEST_CASE(CachedWitnessesDecrementFirst) {
     auto consensusParams = RegtestActivateSapling();
 
+    libzcash::SaplingExtendedSpendingKey sk = GetTestMasterSaplingSpendingKey();
     CWallet& wallet = *pwalletMain;
-    LOCK(wallet.cs_wallet);
-    setupWallet(wallet);
+    {
+        LOCK(wallet.cs_wallet);
+        setupWallet(wallet);
+        BOOST_CHECK(wallet.AddSaplingZKey(sk));
+    }
 
     SaplingMerkleTree saplingTree;
-
-    libzcash::SaplingExtendedSpendingKey sk = GetTestMasterSaplingSpendingKey();
-    BOOST_CHECK(wallet.AddSaplingZKey(sk));
-
     {
         // First block (case tested in _empty_chain)
         CBlock block1;
@@ -799,9 +802,13 @@ BOOST_AUTO_TEST_CASE(CachedWitnessesDecrementFirst) {
 BOOST_AUTO_TEST_CASE(CachedWitnessesCleanIndex) {
     auto consensusParams = RegtestActivateSapling();
 
+    libzcash::SaplingExtendedSpendingKey sk = GetTestMasterSaplingSpendingKey();
     CWallet& wallet = *pwalletMain;
-    LOCK(wallet.cs_wallet);
-    setupWallet(wallet);
+    {
+        LOCK(wallet.cs_wallet);
+        setupWallet(wallet);
+        BOOST_CHECK(wallet.AddSaplingZKey(sk));
+    }
 
     std::vector<CBlock> blocks;
     std::vector<CBlockIndex> indices;
@@ -811,9 +818,6 @@ BOOST_AUTO_TEST_CASE(CachedWitnessesCleanIndex) {
     SaplingMerkleTree saplingRiTree = saplingTree;
     std::vector<Optional<SaplingWitness>> saplingWitnesses;
 
-    libzcash::SaplingExtendedSpendingKey sk = GetTestMasterSaplingSpendingKey();
-    BOOST_CHECK(wallet.AddSaplingZKey(sk));
-
     // Generate a chain
     size_t numBlocks = WITNESS_CACHE_SIZE + 10;
     blocks.resize(numBlocks);
@@ -874,12 +878,13 @@ BOOST_AUTO_TEST_CASE(CachedWitnessesCleanIndex) {
 BOOST_AUTO_TEST_CASE(ClearNoteWitnessCache) {
     auto consensusParams = RegtestActivateSapling();
 
-    CWallet& wallet = *pwalletMain;
-    LOCK(wallet.cs_wallet);
-    setupWallet(wallet);
-
     libzcash::SaplingExtendedSpendingKey sk = GetTestMasterSaplingSpendingKey();
-    BOOST_CHECK(wallet.AddSaplingZKey(sk));
+    CWallet& wallet = *pwalletMain;
+    {
+        LOCK(wallet.cs_wallet);
+        setupWallet(wallet);
+        BOOST_CHECK(wallet.AddSaplingZKey(sk));
+    }
 
     CWalletTx wtx = GetValidSaplingReceive(Params().GetConsensus(),
                                            wallet, sk, 10, true);
@@ -922,7 +927,8 @@ BOOST_AUTO_TEST_CASE(UpdatedSaplingNoteData) {
     auto consensusParams = RegtestActivateSapling();
 
     CWallet& wallet = *pwalletMain;
-    LOCK(wallet.cs_wallet);
+    // Need to lock cs_main for now due the lock ordering. future: revamp all of this function to only lock where is needed.
+    LOCK2(cs_main, wallet.cs_wallet);
     setupWallet(wallet);
 
     auto m = GetTestMasterSaplingSpendingKey();

diff --git a/src/test/librust/utiltest.cpp b/src/test/librust/utiltest.cpp
@@ -43,6 +43,11 @@ CKey AddTestCKeyToKeyStore(CBasicKeyStore& keyStore, bool genNewKey) {
     return tsk;
 }
 
+CKey AddTestCKeyToWallet(CWallet& wallet, bool genNewKey) {
+    LOCK(wallet.cs_wallet);
+    return AddTestCKeyToKeyStore(wallet, genNewKey);
+}
+
 TestSaplingNote GetTestSaplingNote(const libzcash::SaplingPaymentAddress& pa, CAmount value) {
     // Generate dummy Sapling note
     libzcash::SaplingNote note(pa, value);
@@ -80,13 +85,13 @@ CWalletTx GetValidSaplingReceive(const Consensus::Params& consensusParams,
 
 // Two dummy input (to trick coinbase check), one or many shielded outputs
 CWalletTx GetValidSaplingReceive(const Consensus::Params& consensusParams,
-                                 CBasicKeyStore& keyStoreFrom,
+                                 CWallet& keyStoreFrom,
                                  CAmount inputAmount,
                                  std::vector<ShieldedDestination> vDest,
                                  bool genNewKey,
                                  const CWallet* pwalletIn) {
     // From taddr
-    CKey tsk = AddTestCKeyToKeyStore(keyStoreFrom, genNewKey);
+    CKey tsk = AddTestCKeyToWallet(keyStoreFrom, genNewKey);
     auto scriptPubKey = GetScriptForDestination(tsk.GetPubKey().GetID());
 
     // Two equal dummy inputs to by-pass the coinbase check.
@@ -97,7 +102,7 @@ CWalletTx GetValidSaplingReceive(const Consensus::Params& consensusParams,
 
 // Single input, single shielded output
 CWalletTx GetValidSaplingReceive(const Consensus::Params& consensusParams,
-                                 CBasicKeyStore& keyStore,
+                                 CWallet& keyStore,
                                  const libzcash::SaplingExtendedSpendingKey &sk,
                                  CAmount value,
                                  bool genNewKey,

diff --git a/src/test/librust/utiltest.h b/src/test/librust/utiltest.h
@@ -35,6 +35,7 @@ void RegtestDeactivateSapling();
 libzcash::SaplingExtendedSpendingKey GetTestMasterSaplingSpendingKey();
 
 CKey AddTestCKeyToKeyStore(CBasicKeyStore& keyStore, bool genNewKey = false);
+CKey AddTestCKeyToWallet(CWallet& wallet, bool genNewKey = false);
 
 /**
  * Generates a dummy destination script
@@ -60,7 +61,7 @@ CWalletTx GetValidSaplingReceive(const Consensus::Params& consensusParams,
  * Single dummy input, one or many shielded outputs.
  */
 CWalletTx GetValidSaplingReceive(const Consensus::Params& consensusParams,
-                                 CBasicKeyStore& keyStoreFrom,
+                                 CWallet& keyStoreFrom,
                                  CAmount inputAmount,
                                  std::vector<ShieldedDestination> vDest,
                                  bool genNewKey = false,
@@ -70,7 +71,7 @@ CWalletTx GetValidSaplingReceive(const Consensus::Params& consensusParams,
  * Single dummy input, single shielded output to sk default address.
  */
 CWalletTx GetValidSaplingReceive(const Consensus::Params& consensusParams,
-                                 CBasicKeyStore& keyStore,
+                                 CWallet& keyStore,
                                  const libzcash::SaplingExtendedSpendingKey &sk,
                                  CAmount value,
                                  bool genNewKey = false,