From 1886ddafa56517ceab45f8eaaf9725dfb1d87043 Mon Sep 17 00:00:00 2001 From: Fuzzbawls Date: Sat, 17 Apr 2021 18:28:12 -0700 Subject: [PATCH 1/6] [GUI] Fix Cold Staking address list Github-Pull: #2321 Rebased-From: 8afb350a99f4827648f80e4161ba0108d1c8cd26 --- src/qt/pivx/coldstakingwidget.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/qt/pivx/coldstakingwidget.cpp b/src/qt/pivx/coldstakingwidget.cpp index d069b5e4963a..e11af5c960ce 100644 --- a/src/qt/pivx/coldstakingwidget.cpp +++ b/src/qt/pivx/coldstakingwidget.cpp @@ -212,10 +212,10 @@ void ColdStakingWidget::loadWalletModel() addressTableModel = walletModel->getAddressTableModel(); addressesFilter = new AddressFilterProxyModel(AddressTableModel::ColdStaking, this); - addressesFilter->sort(sortType, sortOrder); addressesFilter->setSourceModel(addressTableModel); - ui->listViewStakingAddress->setModelColumn(AddressTableModel::Address); + addressesFilter->sort(sortType, sortOrder); ui->listViewStakingAddress->setModel(addressesFilter); + ui->listViewStakingAddress->setModelColumn(AddressTableModel::Address); connect(txModel, &TransactionTableModel::txArrived, this, &ColdStakingWidget::onTxArrived); From 310db829a35d4f8fc42c8ec957f41af4726721ad Mon Sep 17 00:00:00 2001 From: random-zebra Date: Wed, 21 Apr 2021 16:47:28 +0200 Subject: [PATCH 2/6] [Tests] Add basic unit-test for SaplingScriptPubKeyMan::GetNotes Simple test to trigger an assertion failure. Can be enriched in the future, to check for various configurations. --- src/test/librust/sapling_wallet_tests.cpp | 101 ++++++++++++++++++++++ 1 file changed, 101 insertions(+) diff --git a/src/test/librust/sapling_wallet_tests.cpp b/src/test/librust/sapling_wallet_tests.cpp index af0cc84688ae..1298fbf89f4e 100644 --- a/src/test/librust/sapling_wallet_tests.cpp +++ b/src/test/librust/sapling_wallet_tests.cpp @@ -1148,6 +1148,107 @@ BOOST_AUTO_TEST_CASE(MarkAffectedSaplingTransactionsDirty) { RegtestDeactivateSapling(); } +BOOST_AUTO_TEST_CASE(GetNotes) +{ + auto consensusParams = RegtestActivateSapling(); + + CWallet& wallet = *pwalletMain; + libzcash::SaplingPaymentAddress pk; + uint256 blockHash; + std::vector saplingOutpoints; + { + LOCK2(cs_main, wallet.cs_wallet); + setupWallet(wallet); + + // Generate Sapling address + auto sk = GetTestMasterSaplingSpendingKey(); + auto extfvk = sk.ToXFVK(); + pk = sk.DefaultAddress(); + + BOOST_CHECK(wallet.AddSaplingZKey(sk)); + BOOST_CHECK(wallet.HaveSaplingSpendingKey(extfvk)); + + // Set up transparent address + CBasicKeyStore keystore; + CKey tsk = AddTestCKeyToKeyStore(keystore); + auto scriptPubKey = GetScriptForDestination(tsk.GetPubKey().GetID()); + + // Generate shielding tx from transparent to Sapling (five 1 PIV notes) + auto builder = TransactionBuilder(consensusParams, 1, &keystore); + builder.AddTransparentInput(COutPoint(), scriptPubKey, 510000000); + for (int i=0; i<5; i++) builder.AddSaplingOutput(extfvk.fvk.ovk, pk, 100000000, {}); + builder.SetFee(10000000); + auto tx1 = builder.Build().GetTxOrThrow(); + + CWalletTx wtx {&wallet, MakeTransactionRef(tx1)}; + + // Fake-mine the transaction + BOOST_CHECK_EQUAL(0, chainActive.Height()); + SaplingMerkleTree saplingTree; + CBlock block; + block.vtx.emplace_back(wtx.tx); + block.hashMerkleRoot = BlockMerkleRoot(block); + blockHash = block.GetHash(); + CBlockIndex fakeIndex {block}; + BlockMap::iterator mi = mapBlockIndex.emplace(blockHash, &fakeIndex).first; + fakeIndex.phashBlock = &((*mi).first); + chainActive.SetTip(&fakeIndex); + BOOST_CHECK(chainActive.Contains(&fakeIndex)); + BOOST_CHECK_EQUAL(0, chainActive.Height()); + + // Simulate SyncTransaction which calls AddToWalletIfInvolvingMe + auto saplingNoteData = wallet.GetSaplingScriptPubKeyMan()->FindMySaplingNotes(*wtx.tx).first; + BOOST_CHECK(saplingNoteData.size() > 0); + wtx.SetSaplingNoteData(saplingNoteData); + wtx.m_confirm = CWalletTx::Confirmation(CWalletTx::Status::CONFIRMED, fakeIndex.nHeight, block.GetHash(), 0); + wallet.LoadToWallet(wtx); + + // Simulate receiving new block and ChainTip signal + wallet.IncrementNoteWitnesses(&fakeIndex, &block, saplingTree); + wallet.GetSaplingScriptPubKeyMan()->UpdateSaplingNullifierNoteMapForBlock(&block); + wallet.SetLastBlockProcessed(mi->second); + + const uint256& txid = wtx.GetHash(); + for (int i=0; i<5; i++) saplingOutpoints.emplace_back(txid, i); + } + + // Check GetFilteredNotes + std::vector entries; + Optional address = pk; + wallet.GetSaplingScriptPubKeyMan()->GetFilteredNotes(entries, address, 0, true, false); + for (int i=0; i<5; i++) { + BOOST_CHECK(entries[i].op == saplingOutpoints[i]); + BOOST_CHECK(entries[i].address == pk); + BOOST_CHECK_EQUAL(entries[i].confirmations, 1); + } + + /* + * !TODO: fix GetNotes. + * This test currently fails due to an assertion error (cs_wallet lock not held) + */ + + // Check GetNotes + std::vector entries2; + wallet.GetSaplingScriptPubKeyMan()->GetNotes(saplingOutpoints, entries2); + for (int i=0; i<5; i++) { + BOOST_CHECK(entries2[i].op == entries[i].op); + BOOST_CHECK(entries2[i].address == entries[i].address); + BOOST_CHECK(entries2[i].note.d == entries[i].note.d); + BOOST_CHECK_EQUAL(entries2[i].note.pk_d, entries[i].note.pk_d); + BOOST_CHECK_EQUAL(entries2[i].note.r, entries[i].note.r); + BOOST_CHECK(entries2[i].memo == entries[i].memo); + BOOST_CHECK_EQUAL(entries2[i].confirmations, entries[i].confirmations); + } + + // Tear down + LOCK(cs_main); + chainActive.SetTip(nullptr); + mapBlockIndex.erase(blockHash); + + // Revert to default + RegtestDeactivateSapling(); +} + // TODO: Back port WriteWitnessCache & SetBestChainIgnoresTxsWithoutShieldedData test cases. BOOST_AUTO_TEST_SUITE_END() From 7d7541571b141dd9f5f18a704ce28ed92feee818 Mon Sep 17 00:00:00 2001 From: random-zebra Date: Wed, 21 Apr 2021 15:03:19 +0200 Subject: [PATCH 3/6] [Refactoring] Use CWalletTx::DecryptSaplingNote in Get[Filtered]Notes Remove some code duplication --- src/sapling/saplingscriptpubkeyman.cpp | 42 +++++++++----------------- 1 file changed, 15 insertions(+), 27 deletions(-) diff --git a/src/sapling/saplingscriptpubkeyman.cpp b/src/sapling/saplingscriptpubkeyman.cpp index 3366baee001b..c278affaae5d 100644 --- a/src/sapling/saplingscriptpubkeyman.cpp +++ b/src/sapling/saplingscriptpubkeyman.cpp @@ -399,28 +399,21 @@ void SaplingScriptPubKeyMan::GetNotes(const std::vector& saplin if (!wtx) throw std::runtime_error("No transaction available for hash " + outpoint.hash.GetHex()); const auto& it = wtx->mapSaplingNoteData.find(outpoint); if (it != wtx->mapSaplingNoteData.end()) { - const SaplingOutPoint& op = it->first; const SaplingNoteData& nd = it->second; // skip sent notes if (!nd.IsMyNote()) continue; - const libzcash::SaplingIncomingViewingKey& ivk = *(nd.ivk); - - const OutputDescription& outDesc = wtx->tx->sapData->vShieldedOutput[op.n]; - auto maybe_pt = libzcash::SaplingNotePlaintext::decrypt( - outDesc.encCiphertext, - ivk, - outDesc.ephemeralKey, - outDesc.cmu); - assert(static_cast(maybe_pt)); - auto notePt = maybe_pt.get(); - auto maybe_pa = ivk.address(notePt.d); - assert(static_cast(maybe_pa)); - auto pa = maybe_pa.get(); + // recover plaintext and address + auto optNotePtAndAddress = wtx->DecryptSaplingNote(op); + assert(static_cast(optNotePtAndAddress)); + const libzcash::SaplingIncomingViewingKey& ivk = *(nd.ivk); + const libzcash::SaplingNotePlaintext& notePt = optNotePtAndAddress->first; + const libzcash::SaplingPaymentAddress& pa = optNotePtAndAddress->second; auto note = notePt.note(ivk).get(); + saplingEntriesRet.emplace_back(op, pa, note, notePt.memo(), wtx->GetDepthInMainChain()); } } @@ -481,21 +474,17 @@ void SaplingScriptPubKeyMan::GetFilteredNotes( const SaplingOutPoint& op = it.first; const SaplingNoteData& nd = it.second; - // Skip sent notes + // skip sent notes if (!nd.IsMyNote()) continue; - const libzcash::SaplingIncomingViewingKey& ivk = *(nd.ivk); - auto maybe_pt = libzcash::SaplingNotePlaintext::decrypt( - wtx.tx->sapData->vShieldedOutput[op.n].encCiphertext, - ivk, - wtx.tx->sapData->vShieldedOutput[op.n].ephemeralKey, - wtx.tx->sapData->vShieldedOutput[op.n].cmu); - assert(static_cast(maybe_pt)); - auto notePt = maybe_pt.get(); + // recover plaintext and address + auto optNotePtAndAddress = wtx.DecryptSaplingNote(op); + assert(static_cast(optNotePtAndAddress)); - auto maybe_pa = ivk.address(notePt.d); - assert(static_cast(maybe_pa)); - auto pa = maybe_pa.get(); + const libzcash::SaplingIncomingViewingKey& ivk = *(nd.ivk); + const libzcash::SaplingNotePlaintext& notePt = optNotePtAndAddress->first; + const libzcash::SaplingPaymentAddress& pa = optNotePtAndAddress->second; + auto note = notePt.note(ivk).get(); // skip notes which belong to a different payment address in the wallet if (!(filterAddresses.empty() || filterAddresses.count(pa))) { @@ -516,7 +505,6 @@ void SaplingScriptPubKeyMan::GetFilteredNotes( // continue; //} - auto note = notePt.note(ivk).get(); saplingEntries.emplace_back(op, pa, note, notePt.memo(), wtx.GetDepthInMainChain()); } } From b2d906108ab9dbbebb4c0d4c46d1467b0398f222 Mon Sep 17 00:00:00 2001 From: random-zebra Date: Wed, 21 Apr 2021 15:19:44 +0200 Subject: [PATCH 4/6] [Trivial] Pass big args by const-reference for notes decryption --- src/wallet/wallet.cpp | 5 ++--- src/wallet/wallet.h | 7 +++---- 2 files changed, 5 insertions(+), 7 deletions(-) diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 862a7cd6600e..e7712777cc9b 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -4783,7 +4783,7 @@ void CWalletTx::SetSaplingNoteData(mapSaplingNoteData_t ¬eData) Optional> CWalletTx::DecryptSaplingNote(SaplingOutPoint op) const + libzcash::SaplingPaymentAddress>> CWalletTx::DecryptSaplingNote(const SaplingOutPoint& op) const { // Check whether we can decrypt this SaplingOutPoint with the ivk auto it = this->mapSaplingNoteData.find(op); @@ -4811,8 +4811,7 @@ Optional> CWalletTx::RecoverSaplingNote( - SaplingOutPoint op, std::set& ovks) const + libzcash::SaplingPaymentAddress>> CWalletTx::RecoverSaplingNote(const SaplingOutPoint& op, const std::set& ovks) const { auto output = this->tx->sapData->vShieldedOutput[op.n]; diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index 100221e9b3d8..eb5328365763 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -450,16 +450,15 @@ class CWalletTx void BindWallet(CWallet* pwalletIn); - void SetSaplingNoteData(mapSaplingNoteData_t ¬eData); + void SetSaplingNoteData(mapSaplingNoteData_t& noteData); Optional> DecryptSaplingNote(SaplingOutPoint op) const; + libzcash::SaplingPaymentAddress>> DecryptSaplingNote(const SaplingOutPoint& op) const; Optional> RecoverSaplingNote( - SaplingOutPoint op, std::set& ovks) const; + libzcash::SaplingPaymentAddress>> RecoverSaplingNote(const SaplingOutPoint& op, const std::set& ovks) const; //! checks whether a tx has P2CS inputs or not bool HasP2CSInputs() const; From 9bec9bc2f15fe58778d922dde24130ebf2f7523d Mon Sep 17 00:00:00 2001 From: random-zebra Date: Wed, 21 Apr 2021 15:58:29 +0200 Subject: [PATCH 5/6] [BUG] Missing cs_wallet lock in SaplingScriptPubKeyMan::GetNotes needed by CWalletTx::GetDepthInMainChain --- src/sapling/saplingscriptpubkeyman.cpp | 3 ++- src/test/librust/sapling_wallet_tests.cpp | 5 ----- 2 files changed, 2 insertions(+), 6 deletions(-) diff --git a/src/sapling/saplingscriptpubkeyman.cpp b/src/sapling/saplingscriptpubkeyman.cpp index c278affaae5d..eb7ac3c036fb 100644 --- a/src/sapling/saplingscriptpubkeyman.cpp +++ b/src/sapling/saplingscriptpubkeyman.cpp @@ -397,6 +397,7 @@ void SaplingScriptPubKeyMan::GetNotes(const std::vector& saplin for (const auto& outpoint : saplingOutpoints) { const auto* wtx = wallet->GetWalletTx(outpoint.hash); if (!wtx) throw std::runtime_error("No transaction available for hash " + outpoint.hash.GetHex()); + const int depth = WITH_LOCK(wallet->cs_wallet, return wtx->GetDepthInMainChain(); ); const auto& it = wtx->mapSaplingNoteData.find(outpoint); if (it != wtx->mapSaplingNoteData.end()) { const SaplingOutPoint& op = it->first; @@ -414,7 +415,7 @@ void SaplingScriptPubKeyMan::GetNotes(const std::vector& saplin const libzcash::SaplingPaymentAddress& pa = optNotePtAndAddress->second; auto note = notePt.note(ivk).get(); - saplingEntriesRet.emplace_back(op, pa, note, notePt.memo(), wtx->GetDepthInMainChain()); + saplingEntriesRet.emplace_back(op, pa, note, notePt.memo(), depth); } } } diff --git a/src/test/librust/sapling_wallet_tests.cpp b/src/test/librust/sapling_wallet_tests.cpp index 1298fbf89f4e..d0f4e3da6c66 100644 --- a/src/test/librust/sapling_wallet_tests.cpp +++ b/src/test/librust/sapling_wallet_tests.cpp @@ -1222,11 +1222,6 @@ BOOST_AUTO_TEST_CASE(GetNotes) BOOST_CHECK_EQUAL(entries[i].confirmations, 1); } - /* - * !TODO: fix GetNotes. - * This test currently fails due to an assertion error (cs_wallet lock not held) - */ - // Check GetNotes std::vector entries2; wallet.GetSaplingScriptPubKeyMan()->GetNotes(saplingOutpoints, entries2); From 68b91b1dc6c43ca6032f979fd3e772dd179cc8a3 Mon Sep 17 00:00:00 2001 From: random-zebra Date: Wed, 21 Apr 2021 16:05:20 +0200 Subject: [PATCH 6/6] [Refactoring] Don't compute depth multiple times in GetFilteredNotes --- src/sapling/saplingscriptpubkeyman.cpp | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/sapling/saplingscriptpubkeyman.cpp b/src/sapling/saplingscriptpubkeyman.cpp index eb7ac3c036fb..769cbd5f8bf6 100644 --- a/src/sapling/saplingscriptpubkeyman.cpp +++ b/src/sapling/saplingscriptpubkeyman.cpp @@ -465,9 +465,9 @@ void SaplingScriptPubKeyMan::GetFilteredNotes( } // Filter the transactions before checking for notes + const int depth = wtx.GetDepthInMainChain(); if (!IsFinalTx(wtx.tx, wallet->GetLastBlockHeight() + 1, GetAdjustedTime()) || - wtx.GetDepthInMainChain() < minDepth || - wtx.GetDepthInMainChain() > maxDepth) { + depth < minDepth || depth > maxDepth) { continue; } @@ -506,7 +506,7 @@ void SaplingScriptPubKeyMan::GetFilteredNotes( // continue; //} - saplingEntries.emplace_back(op, pa, note, notePt.memo(), wtx.GetDepthInMainChain()); + saplingEntries.emplace_back(op, pa, note, notePt.memo(), depth); } } }