From 0de659946e949935b234fa8aa7e07b5c6b9ee739 Mon Sep 17 00:00:00 2001 From: Jack Grigg Date: Wed, 28 Nov 2018 00:56:42 +0100 Subject: [PATCH 1/4] wallet: Skip transactions with no shielded data in CWallet::SetBestChain() Co-authored-by: Daira Hopwood Closes #3495. --- src/wallet/gtest/test_wallet.cpp | 57 ++++++++++++++++++++++++++++++++ src/wallet/wallet.h | 15 ++++++--- 2 files changed, 68 insertions(+), 4 deletions(-) diff --git a/src/wallet/gtest/test_wallet.cpp b/src/wallet/gtest/test_wallet.cpp index f4b9ddc55..aed3f1184 100644 --- a/src/wallet/gtest/test_wallet.cpp +++ b/src/wallet/gtest/test_wallet.cpp @@ -1627,6 +1627,63 @@ TEST(WalletTests, WriteWitnessCache) { wallet.SetBestChain(walletdb, loc); } +TEST(WalletTests, SetBestChainIgnoresTxsWithoutShieldedData) { + SelectParams(CBaseChainParams::REGTEST); + + TestWallet wallet; + MockWalletDB walletdb; + CBlockLocator loc; + + // Set up transparent address + CKey tsk = DecodeSecret(tSecretRegtest); + wallet.AddKey(tsk); + auto scriptPubKey = GetScriptForDestination(tsk.GetPubKey().GetID()); + + // Set up a Sprout address + auto sk = libzcash::SproutSpendingKey::random(); + wallet.AddSproutSpendingKey(sk); + + // Generate a transparent transaction that is ours + CMutableTransaction t; + t.vout.resize(1); + t.vout[0].nValue = 90*CENT; + t.vout[0].scriptPubKey = scriptPubKey; + CWalletTx wtxTransparent {nullptr, t}; + wallet.AddToWallet(wtxTransparent, true, NULL); + + // Generate a Sprout transaction that is ours + auto wtxSprout = GetValidReceive(sk, 10, true); + auto noteMap = wallet.FindMySproutNotes(wtxSprout); + wtxSprout.SetSproutNoteData(noteMap); + wallet.AddToWallet(wtxSprout, true, NULL); + + // Generate a Sprout transaction that only involves our transparent address + auto sk2 = libzcash::SproutSpendingKey::random(); + auto wtxInput = GetValidReceive(sk2, 10, true); + auto note = GetNote(sk2, wtxInput, 0, 0); + auto wtxTmp = GetValidSpend(sk2, note, 5); + CMutableTransaction mtx {wtxTmp}; + mtx.vout[0].scriptPubKey = scriptPubKey; + CWalletTx wtxSproutTransparent {NULL, mtx}; + wallet.AddToWallet(wtxSproutTransparent, true, NULL); + + EXPECT_CALL(walletdb, TxnBegin()) + .WillOnce(Return(true)); + EXPECT_CALL(walletdb, WriteTx(wtxTransparent.GetHash(), wtxTransparent)) + .Times(0); + EXPECT_CALL(walletdb, WriteTx(wtxSprout.GetHash(), wtxSprout)) + .Times(1).WillOnce(Return(true)); + EXPECT_CALL(walletdb, WriteTx(wtxSproutTransparent.GetHash(), wtxSproutTransparent)) + .Times(0); + EXPECT_CALL(walletdb, WriteWitnessCacheSize(0)) + .WillOnce(Return(true)); + EXPECT_CALL(walletdb, WriteBestBlock(loc)) + .WillOnce(Return(true)); + EXPECT_CALL(walletdb, TxnCommit()) + .WillOnce(Return(true)); + wallet.SetBestChain(walletdb, loc); +} + TEST(WalletTests, UpdateSproutNullifierNoteMap) { TestWallet wallet; uint256 r {GetRandHash()}; diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index ef7baa465..b24e76b19 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -790,10 +790,17 @@ protected: } try { for (std::pair& wtxItem : mapWallet) { - if (!walletdb.WriteTx(wtxItem.first, wtxItem.second)) { - LogPrintf("SetBestChain(): Failed to write CWalletTx, aborting atomic write\n"); - walletdb.TxnAbort(); - return; + auto wtx = wtxItem.second; + // We skip transactions for which mapSproutNoteData and mapSaplingNoteData + // are empty. This covers transactions that have no Sprout or Sapling data + // (i.e. are purely transparent), as well as shielding and unshielding + // transactions in which we only have transparent addresses involved. + if (!(wtx.mapSproutNoteData.empty() && wtx.mapSaplingNoteData.empty())) { + if (!walletdb.WriteTx(wtxItem.first, wtx)) { + LogPrintf("SetBestChain(): Failed to write CWalletTx, aborting atomic write\n"); + walletdb.TxnAbort(); + return; + } } } if (!walletdb.WriteWitnessCacheSize(nWitnessCacheSize)) { From c3eaffdb48ae41222e3d6de21460c73074a6f661 Mon Sep 17 00:00:00 2001 From: Jack Grigg Date: Sat, 1 Dec 2018 15:51:00 +0000 Subject: [PATCH 2/4] Use nullptr instead of NULL in wallet tests --- src/wallet/gtest/test_wallet.cpp | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/wallet/gtest/test_wallet.cpp b/src/wallet/gtest/test_wallet.cpp index aed3f1184..f57653ad1 100644 --- a/src/wallet/gtest/test_wallet.cpp +++ b/src/wallet/gtest/test_wallet.cpp @@ -1649,13 +1649,13 @@ TEST(WalletTests, SetBestChainIgnoresTxsWithoutShieldedData) { t.vout[0].nValue = 90*CENT; t.vout[0].scriptPubKey = scriptPubKey; CWalletTx wtxTransparent {nullptr, t}; - wallet.AddToWallet(wtxTransparent, true, NULL); + wallet.AddToWallet(wtxTransparent, true, nullptr); // Generate a Sprout transaction that is ours auto wtxSprout = GetValidReceive(sk, 10, true); auto noteMap = wallet.FindMySproutNotes(wtxSprout); wtxSprout.SetSproutNoteData(noteMap); - wallet.AddToWallet(wtxSprout, true, NULL); + wallet.AddToWallet(wtxSprout, true, nullptr); // Generate a Sprout transaction that only involves our transparent address auto sk2 = libzcash::SproutSpendingKey::random(); @@ -1664,8 +1664,8 @@ TEST(WalletTests, SetBestChainIgnoresTxsWithoutShieldedData) { auto wtxTmp = GetValidSpend(sk2, note, 5); CMutableTransaction mtx {wtxTmp}; mtx.vout[0].scriptPubKey = scriptPubKey; - CWalletTx wtxSproutTransparent {NULL, mtx}; - wallet.AddToWallet(wtxSproutTransparent, true, NULL); + CWalletTx wtxSproutTransparent {nullptr, mtx}; + wallet.AddToWallet(wtxSproutTransparent, true, nullptr); EXPECT_CALL(walletdb, TxnBegin()) .WillOnce(Return(true)); From 7afb23ef31080d03216716d3d543cd3936b70e5a Mon Sep 17 00:00:00 2001 From: Jack Grigg Date: Sat, 1 Dec 2018 15:55:34 +0000 Subject: [PATCH 3/4] Add Sapling test cases --- src/wallet/gtest/test_wallet.cpp | 25 +++++++++++++++++++++++++ 1 file changed, 25 insertions(+) diff --git a/src/wallet/gtest/test_wallet.cpp b/src/wallet/gtest/test_wallet.cpp index f57653ad1..bf9e0c139 100644 --- a/src/wallet/gtest/test_wallet.cpp +++ b/src/wallet/gtest/test_wallet.cpp @@ -1667,6 +1667,27 @@ TEST(WalletTests, SetBestChainIgnoresTxsWithoutShieldedData) { CWalletTx wtxSproutTransparent {nullptr, mtx}; wallet.AddToWallet(wtxSproutTransparent, true, nullptr); + // Generate a fake Sapling transaction + CMutableTransaction mtxSapling; + mtxSapling.fOverwintered = true; + mtxSapling.nVersion = SAPLING_TX_VERSION; + mtxSapling.nVersionGroupId = SAPLING_VERSION_GROUP_ID; + mtxSapling.vShieldedOutput.resize(1); + mtxSapling.vShieldedOutput[0].cv = libzcash::random_uint256(); + CWalletTx wtxSapling {nullptr, mtxSapling}; + SetSaplingNoteData(wtxSapling); + wallet.AddToWallet(wtxSapling, true, nullptr); + + // Generate a fake Sapling transaction that would only involve our transparent addresses + CMutableTransaction mtxSaplingTransparent; + mtxSaplingTransparent.fOverwintered = true; + mtxSaplingTransparent.nVersion = SAPLING_TX_VERSION; + mtxSaplingTransparent.nVersionGroupId = SAPLING_VERSION_GROUP_ID; + mtxSaplingTransparent.vShieldedOutput.resize(1); + mtxSaplingTransparent.vShieldedOutput[0].cv = libzcash::random_uint256(); + CWalletTx wtxSaplingTransparent {nullptr, mtxSaplingTransparent}; + wallet.AddToWallet(wtxSaplingTransparent, true, nullptr); + EXPECT_CALL(walletdb, TxnBegin()) .WillOnce(Return(true)); EXPECT_CALL(walletdb, WriteTx(wtxTransparent.GetHash(), wtxTransparent)) @@ -1675,6 +1696,10 @@ TEST(WalletTests, SetBestChainIgnoresTxsWithoutShieldedData) { .Times(1).WillOnce(Return(true)); EXPECT_CALL(walletdb, WriteTx(wtxSproutTransparent.GetHash(), wtxSproutTransparent)) .Times(0); + EXPECT_CALL(walletdb, WriteTx(wtxSapling.GetHash(), wtxSapling)) + .Times(1).WillOnce(Return(true)); + EXPECT_CALL(walletdb, WriteTx(wtxSaplingTransparent.GetHash(), wtxSaplingTransparent)) + .Times(0); EXPECT_CALL(walletdb, WriteWitnessCacheSize(0)) .WillOnce(Return(true)); EXPECT_CALL(walletdb, WriteBestBlock(loc)) From a6da0b03dab684c89504dc1ec761ea47a8af3539 Mon Sep 17 00:00:00 2001 From: Jack Grigg Date: Wed, 2 Jan 2019 13:23:28 +0000 Subject: [PATCH 4/4] Set Sprout note data in WalletTest.WriteWitnessCache --- src/wallet/gtest/test_wallet.cpp | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/src/wallet/gtest/test_wallet.cpp b/src/wallet/gtest/test_wallet.cpp index bf9e0c139..7c7fdb404 100644 --- a/src/wallet/gtest/test_wallet.cpp +++ b/src/wallet/gtest/test_wallet.cpp @@ -1559,6 +1559,14 @@ TEST(WalletTests, WriteWitnessCache) { wallet.AddSproutSpendingKey(sk); auto wtx = GetValidReceive(sk, 10, true); + auto note = GetNote(sk, wtx, 0, 1); + auto nullifier = note.nullifier(sk); + + mapSproutNoteData_t noteData; + JSOutPoint jsoutpt {wtx.GetHash(), 0, 1}; + SproutNoteData nd {sk.address(), nullifier}; + noteData[jsoutpt] = nd; + wtx.SetSproutNoteData(noteData); wallet.AddToWallet(wtx, true, NULL); // TxnBegin fails