Skip to content

Commit

Permalink
Merge #2327: [BUG] Fix crash when sending shield notes from the GUI w…
Browse files Browse the repository at this point in the history 
…ith coin control

dcc0166 [Refactoring] Don't compute depth multiple times in GetFilteredNotes (random-zebra)
85c7425 [BUG] Missing cs_wallet lock in SaplingScriptPubKeyMan::GetNotes (random-zebra)
fbb7813 [Trivial] Pass big args by const-reference for notes decryption (random-zebra)
28cf3fb [Refactoring] Use CWalletTx::DecryptSaplingNote in Get[Filtered]Notes (random-zebra)
c316dda [Tests] Add basic unit-test for SaplingScriptPubKeyMan::GetNotes (random-zebra)

Pull request description:

  This particular flow calls `SaplingScriptPubKeyMan::GetNotes` (instead of `GetFilteredNotes`), which calls `CWalletTx::GetDepthInMainChain` without holding `cs_wallet` (requirement introduced via `AssertLockHeld` in 1386ab7).

  Add a simple unittest to cover `GetNotes`/`GetFilteredNotes`, do a bit of refactoring (removing some code duplication), and fix the missing lock.

ACKs for top commit:
  furszy:
    ACK dcc0166 .
  Fuzzbawls:
    ACK dcc0166

Tree-SHA512: a33ff8d8cb71b845c056f00eb9309292d10937ea3e18c71b402a907905bf305a61061c0eeedd27342001faf6d26c06430cbe34e0f555da4dfe052817d5506330
  • Loading branch information
@random-zebra
random-zebra committed Apr 23, 2021
2 parents 0beffdb + dcc0166 commit d38a486
Show file tree
Hide file tree
Showing 4 changed files with 121 additions and 38 deletions.
51 changes: 20 additions & 31 deletions src/sapling/saplingscriptpubkeyman.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -397,31 +397,25 @@ void SaplingScriptPubKeyMan::GetNotes(const std::vector<SaplingOutPoint>& 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;
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<bool>(maybe_pt));
auto notePt = maybe_pt.get();

auto maybe_pa = ivk.address(notePt.d);
assert(static_cast<bool>(maybe_pa));
auto pa = maybe_pa.get();
// recover plaintext and address
auto optNotePtAndAddress = wtx->DecryptSaplingNote(op);
assert(static_cast<bool>(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());

saplingEntriesRet.emplace_back(op, pa, note, notePt.memo(), depth);
}
}
}
Expand Down Expand Up @@ -471,31 +465,27 @@ 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;
}

for (const auto& it : wtx.mapSaplingNoteData) {
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<bool>(maybe_pt));
auto notePt = maybe_pt.get();
// recover plaintext and address
auto optNotePtAndAddress = wtx.DecryptSaplingNote(op);
assert(static_cast<bool>(optNotePtAndAddress));

auto maybe_pa = ivk.address(notePt.d);
assert(static_cast<bool>(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))) {
Expand All @@ -516,8 +506,7 @@ void SaplingScriptPubKeyMan::GetFilteredNotes(
// continue;
//}

auto note = notePt.note(ivk).get();
saplingEntries.emplace_back(op, pa, note, notePt.memo(), wtx.GetDepthInMainChain());
saplingEntries.emplace_back(op, pa, note, notePt.memo(), depth);
}
}
}
Expand Down
96 changes: 96 additions & 0 deletions src/test/librust/sapling_wallet_tests.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1147,6 +1147,102 @@ BOOST_AUTO_TEST_CASE(MarkAffectedSaplingTransactionsDirty) {
RegtestDeactivateSapling();
}

BOOST_AUTO_TEST_CASE(GetNotes)
{
auto consensusParams = RegtestActivateSapling();

CWallet& wallet = *pwalletMain;
libzcash::SaplingPaymentAddress pk;
uint256 blockHash;
std::vector<SaplingOutPoint> 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<SaplingNoteEntry> entries;
Optional<libzcash::SaplingPaymentAddress> 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);
}

// Check GetNotes
std::vector<SaplingNoteEntry> 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()
5 changes: 2 additions & 3 deletions src/wallet/wallet.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4755,7 +4755,7 @@ void CWalletTx::SetSaplingNoteData(mapSaplingNoteData_t &noteData)

Optional<std::pair<
libzcash::SaplingNotePlaintext,
libzcash::SaplingPaymentAddress>> 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);
Expand Down Expand Up @@ -4783,8 +4783,7 @@ Optional<std::pair<

Optional<std::pair<
libzcash::SaplingNotePlaintext,
libzcash::SaplingPaymentAddress>> CWalletTx::RecoverSaplingNote(
SaplingOutPoint op, std::set<uint256>& ovks) const
libzcash::SaplingPaymentAddress>> CWalletTx::RecoverSaplingNote(const SaplingOutPoint& op, const std::set<uint256>& ovks) const
{
auto output = this->tx->sapData->vShieldedOutput[op.n];

Expand Down
7 changes: 3 additions & 4 deletions src/wallet/wallet.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -452,16 +452,15 @@ class CWalletTx

void BindWallet(CWallet* pwalletIn);

void SetSaplingNoteData(mapSaplingNoteData_t &noteData);
void SetSaplingNoteData(mapSaplingNoteData_t& noteData);

Optional<std::pair<
libzcash::SaplingNotePlaintext,
libzcash::SaplingPaymentAddress>> DecryptSaplingNote(SaplingOutPoint op) const;
libzcash::SaplingPaymentAddress>> DecryptSaplingNote(const SaplingOutPoint& op) const;

Optional<std::pair<
libzcash::SaplingNotePlaintext,
libzcash::SaplingPaymentAddress>> RecoverSaplingNote(
SaplingOutPoint op, std::set<uint256>& ovks) const;
libzcash::SaplingPaymentAddress>> RecoverSaplingNote(const SaplingOutPoint& op, const std::set<uint256>& ovks) const;

//! checks whether a tx has P2CS inputs or not
bool HasP2CSInputs() const;
Expand Down

0 comments on commit d38a486

Please sign in to comment.