Skip to content

Commit e0a0d2d

Browse files
furszyfurszy
committed
sapling_wallet_tests: locking order refactor, solving inconsistent orders for cs_main and cs_wallet.
1 parent c69e7e8 commit e0a0d2d

File tree

3 files changed

+42
-30
lines changed

3 files changed

+42
-30
lines changed

‎src/test/librust/sapling_wallet_tests.cpp‎

Lines changed: 31 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -614,8 +614,10 @@ BOOST_AUTO_TEST_CASE(CachedWitnessesEmptyChain) {
614614
auto consensusParams = RegtestActivateSapling();
615615

616616
CWallet& wallet = *pwalletMain;
617-
LOCK(wallet.cs_wallet);
618-
setupWallet(wallet);
617+
{
618+
LOCK(wallet.cs_wallet);
619+
setupWallet(wallet);
620+
}
619621

620622
auto sk = GetTestMasterSaplingSpendingKey();
621623
CWalletTx wtx = GetValidSaplingReceive(Params().GetConsensus(), wallet, sk, 10, true);
@@ -653,18 +655,19 @@ BOOST_AUTO_TEST_CASE(CachedWitnessesEmptyChain) {
653655
BOOST_AUTO_TEST_CASE(CachedWitnessesChainTip) {
654656
auto consensusParams = RegtestActivateSapling();
655657

658+
libzcash::SaplingExtendedSpendingKey sk = GetTestMasterSaplingSpendingKey();
656659
CWallet& wallet = *pwalletMain;
657-
LOCK(wallet.cs_wallet);
658-
setupWallet(wallet);
660+
{
661+
LOCK(wallet.cs_wallet);
662+
setupWallet(wallet);
663+
BOOST_CHECK(wallet.AddSaplingZKey(sk));
664+
BOOST_CHECK(wallet.HaveSaplingSpendingKey(sk.ToXFVK()));
665+
}
659666

660667
uint256 anchors1;
661668
CBlock block1;
662669
SaplingMerkleTree saplingTree;
663670

664-
libzcash::SaplingExtendedSpendingKey sk = GetTestMasterSaplingSpendingKey();
665-
BOOST_CHECK(wallet.AddSaplingZKey(sk));
666-
BOOST_CHECK(wallet.HaveSaplingSpendingKey(sk.ToXFVK()));
667-
668671
{
669672
// First block (case tested in _empty_chain)
670673
CBlockIndex index1(block1);
@@ -731,15 +734,15 @@ BOOST_AUTO_TEST_CASE(CachedWitnessesChainTip) {
731734
BOOST_AUTO_TEST_CASE(CachedWitnessesDecrementFirst) {
732735
auto consensusParams = RegtestActivateSapling();
733736

737+
libzcash::SaplingExtendedSpendingKey sk = GetTestMasterSaplingSpendingKey();
734738
CWallet& wallet = *pwalletMain;
735-
LOCK(wallet.cs_wallet);
736-
setupWallet(wallet);
739+
{
740+
LOCK(wallet.cs_wallet);
741+
setupWallet(wallet);
742+
BOOST_CHECK(wallet.AddSaplingZKey(sk));
743+
}
737744

738745
SaplingMerkleTree saplingTree;
739-
740-
libzcash::SaplingExtendedSpendingKey sk = GetTestMasterSaplingSpendingKey();
741-
BOOST_CHECK(wallet.AddSaplingZKey(sk));
742-
743746
{
744747
// First block (case tested in _empty_chain)
745748
CBlock block1;
@@ -799,9 +802,13 @@ BOOST_AUTO_TEST_CASE(CachedWitnessesDecrementFirst) {
799802
BOOST_AUTO_TEST_CASE(CachedWitnessesCleanIndex) {
800803
auto consensusParams = RegtestActivateSapling();
801804

805+
libzcash::SaplingExtendedSpendingKey sk = GetTestMasterSaplingSpendingKey();
802806
CWallet& wallet = *pwalletMain;
803-
LOCK(wallet.cs_wallet);
804-
setupWallet(wallet);
807+
{
808+
LOCK(wallet.cs_wallet);
809+
setupWallet(wallet);
810+
BOOST_CHECK(wallet.AddSaplingZKey(sk));
811+
}
805812

806813
std::vector<CBlock> blocks;
807814
std::vector<CBlockIndex> indices;
@@ -811,9 +818,6 @@ BOOST_AUTO_TEST_CASE(CachedWitnessesCleanIndex) {
811818
SaplingMerkleTree saplingRiTree = saplingTree;
812819
std::vector<Optional<SaplingWitness>> saplingWitnesses;
813820

814-
libzcash::SaplingExtendedSpendingKey sk = GetTestMasterSaplingSpendingKey();
815-
BOOST_CHECK(wallet.AddSaplingZKey(sk));
816-
817821
// Generate a chain
818822
size_t numBlocks = WITNESS_CACHE_SIZE + 10;
819823
blocks.resize(numBlocks);
@@ -874,12 +878,13 @@ BOOST_AUTO_TEST_CASE(CachedWitnessesCleanIndex) {
874878
BOOST_AUTO_TEST_CASE(ClearNoteWitnessCache) {
875879
auto consensusParams = RegtestActivateSapling();
876880

877-
CWallet& wallet = *pwalletMain;
878-
LOCK(wallet.cs_wallet);
879-
setupWallet(wallet);
880-
881881
libzcash::SaplingExtendedSpendingKey sk = GetTestMasterSaplingSpendingKey();
882-
BOOST_CHECK(wallet.AddSaplingZKey(sk));
882+
CWallet& wallet = *pwalletMain;
883+
{
884+
LOCK(wallet.cs_wallet);
885+
setupWallet(wallet);
886+
BOOST_CHECK(wallet.AddSaplingZKey(sk));
887+
}
883888

884889
CWalletTx wtx = GetValidSaplingReceive(Params().GetConsensus(),
885890
wallet, sk, 10, true);
@@ -922,7 +927,8 @@ BOOST_AUTO_TEST_CASE(UpdatedSaplingNoteData) {
922927
auto consensusParams = RegtestActivateSapling();
923928

924929
CWallet& wallet = *pwalletMain;
925-
LOCK(wallet.cs_wallet);
930+
// Need to lock cs_main for now due the lock ordering. future: revamp all of this function to only lock where is needed.
931+
LOCK2(cs_main, wallet.cs_wallet);
926932
setupWallet(wallet);
927933

928934
auto m = GetTestMasterSaplingSpendingKey();

‎src/test/librust/utiltest.cpp‎

Lines changed: 8 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -43,6 +43,11 @@ CKey AddTestCKeyToKeyStore(CBasicKeyStore& keyStore, bool genNewKey) {
4343
return tsk;
4444
}
4545

46+
CKey AddTestCKeyToWallet(CWallet& wallet, bool genNewKey) {
47+
LOCK(wallet.cs_wallet);
48+
return AddTestCKeyToKeyStore(wallet, genNewKey);
49+
}
50+
4651
TestSaplingNote GetTestSaplingNote(const libzcash::SaplingPaymentAddress& pa, CAmount value) {
4752
// Generate dummy Sapling note
4853
libzcash::SaplingNote note(pa, value);
@@ -80,13 +85,13 @@ CWalletTx GetValidSaplingReceive(const Consensus::Params& consensusParams,
8085

8186
// Two dummy input (to trick coinbase check), one or many shielded outputs
8287
CWalletTx GetValidSaplingReceive(const Consensus::Params& consensusParams,
83-
CBasicKeyStore& keyStoreFrom,
88+
CWallet& keyStoreFrom,
8489
CAmount inputAmount,
8590
std::vector<ShieldedDestination> vDest,
8691
bool genNewKey,
8792
const CWallet* pwalletIn) {
8893
// From taddr
89-
CKey tsk = AddTestCKeyToKeyStore(keyStoreFrom, genNewKey);
94+
CKey tsk = AddTestCKeyToWallet(keyStoreFrom, genNewKey);
9095
auto scriptPubKey = GetScriptForDestination(tsk.GetPubKey().GetID());
9196

9297
// Two equal dummy inputs to by-pass the coinbase check.
@@ -97,7 +102,7 @@ CWalletTx GetValidSaplingReceive(const Consensus::Params& consensusParams,
97102

98103
// Single input, single shielded output
99104
CWalletTx GetValidSaplingReceive(const Consensus::Params& consensusParams,
100-
CBasicKeyStore& keyStore,
105+
CWallet& keyStore,
101106
const libzcash::SaplingExtendedSpendingKey &sk,
102107
CAmount value,
103108
bool genNewKey,

‎src/test/librust/utiltest.h‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -35,6 +35,7 @@ void RegtestDeactivateSapling();
3535
libzcash::SaplingExtendedSpendingKey GetTestMasterSaplingSpendingKey();
3636

3737
CKey AddTestCKeyToKeyStore(CBasicKeyStore& keyStore, bool genNewKey = false);
38+
CKey AddTestCKeyToWallet(CWallet& wallet, bool genNewKey = false);
3839

3940
/**
4041
* Generates a dummy destination script
@@ -60,7 +61,7 @@ CWalletTx GetValidSaplingReceive(const Consensus::Params& consensusParams,
6061
* Single dummy input, one or many shielded outputs.
6162
*/
6263
CWalletTx GetValidSaplingReceive(const Consensus::Params& consensusParams,
63-
CBasicKeyStore& keyStoreFrom,
64+
CWallet& keyStoreFrom,
6465
CAmount inputAmount,
6566
std::vector<ShieldedDestination> vDest,
6667
bool genNewKey = false,
@@ -70,7 +71,7 @@ CWalletTx GetValidSaplingReceive(const Consensus::Params& consensusParams,
7071
* Single dummy input, single shielded output to sk default address.
7172
*/
7273
CWalletTx GetValidSaplingReceive(const Consensus::Params& consensusParams,
73-
CBasicKeyStore& keyStore,
74+
CWallet& keyStore,
7475
const libzcash::SaplingExtendedSpendingKey &sk,
7576
CAmount value,
7677
bool genNewKey = false,

0 commit comments

Comments
 (0)