From bd6a34b0eae9b582391c1c0d6af46bbbd6840334 Mon Sep 17 00:00:00 2001 From: therealyingtong Date: Thu, 10 Mar 2022 16:27:28 +0800 Subject: [PATCH 1/5] test_wallet: Test GetConflictedOrchardNotes. --- src/wallet/gtest/test_wallet.cpp | 121 +++++++++++++++++++++++++++++++ 1 file changed, 121 insertions(+) diff --git a/src/wallet/gtest/test_wallet.cpp b/src/wallet/gtest/test_wallet.cpp index a1d5a8ffd..2e8a992d5 100644 --- a/src/wallet/gtest/test_wallet.cpp +++ b/src/wallet/gtest/test_wallet.cpp @@ -806,6 +806,127 @@ TEST(WalletTests, GetConflictedSaplingNotes) { } } +TEST(WalletTests, GetConflictedOrchardNotes) { + LoadProofParameters(); + + auto consensusParams = RegtestActivateNU5(); + TestWallet wallet(Params()); + wallet.GenerateNewSeed(); + + LOCK2(cs_main, wallet.cs_wallet); + + // Create an account. + auto ufvk = wallet.GenerateNewUnifiedSpendingKey().first; + auto fvk = ufvk.GetOrchardKey().value(); + auto ivk = fvk.ToIncomingViewingKey(); + libzcash::diversifier_index_t j(0); + auto recipient = ivk.Address(j); + + uint256 orchardAnchor; + orchard::Builder(true, true, orchardAnchor); + + // Generate transparent funds + CBasicKeyStore keystore; + CKey tsk = AddTestCKeyToKeyStore(keystore); + auto tkeyid = tsk.GetPubKey().GetID(); + auto scriptPubKey = GetScriptForDestination(tkeyid); + + // Generate a bundle containing output note A. + auto builder = TransactionBuilder(consensusParams, 1, orchardAnchor, &keystore); + builder.AddTransparentInput(COutPoint(), scriptPubKey, 50000); + builder.AddOrchardOutput(std::nullopt, recipient, 40000, {}); + auto tx = builder.Build().GetTxOrThrow(); + CWalletTx wtx {&wallet, tx}; + + // Fake-mine the transaction + SproutMerkleTree sproutTree; + SaplingMerkleTree saplingTree; + OrchardMerkleFrontier orchardTree; + orchardTree.AppendBundle(wtx.GetOrchardBundle()); + + EXPECT_EQ(-1, chainActive.Height()); + CBlock block; + block.vtx.push_back(wtx); + block.hashMerkleRoot = block.BuildMerkleTree(); + auto blockHash = block.GetHash(); + CBlockIndex fakeIndex {block}; + fakeIndex.hashFinalOrchardRoot = orchardTree.root(); + mapBlockIndex.insert(std::make_pair(blockHash, &fakeIndex)); + chainActive.SetTip(&fakeIndex); + EXPECT_TRUE(chainActive.Contains(&fakeIndex)); + EXPECT_EQ(0, chainActive.Height()); + + // Simulate SyncTransaction which calls AddToWalletIfInvolvingMe + auto orchardTxMeta = wallet.GetOrchardWallet().AddNotesIfInvolvingMe(wtx); + ASSERT_TRUE(orchardTxMeta.has_value()); + EXPECT_FALSE(orchardTxMeta.value().empty()); + wtx.SetOrchardTxMeta(orchardTxMeta.value()); + wtx.SetMerkleBranch(block); + wallet.LoadWalletTx(wtx); + + // Simulate receiving new block and ChainTip signal + wallet.IncrementNoteWitnesses(Params().GetConsensus(),&fakeIndex, &block, sproutTree, saplingTree, true); + wallet.UpdateSaplingNullifierNoteMapForBlock(&block); + + // Fetch the Orchard note so we can spend it. + std::vector sproutEntries; + std::vector saplingEntries; + std::vector orchardEntries; + wallet.GetFilteredNotes(sproutEntries, saplingEntries, orchardEntries, std::nullopt, -1); + EXPECT_EQ(0, sproutEntries.size()); + EXPECT_EQ(0, saplingEntries.size()); + EXPECT_EQ(1, orchardEntries.size()); + + // Generate another recipient + libzcash::diversifier_index_t j2(1); + auto recipient2 = ivk.Address(j2); + + // Generate tx to spend note A + auto noteToSpend = std::move(wallet.GetOrchardSpendInfo(orchardEntries)[0]); + auto builder2 = TransactionBuilder(consensusParams, 2, orchardTree.root()); + builder2.AddOrchardSpend(std::move(noteToSpend.first), std::move(noteToSpend.second)); + auto tx2 = builder2.Build().GetTxOrThrow(); + CWalletTx wtx2 {&wallet, tx2}; + + // Generate conflicting tx to spend note A + auto noteToSpend2 = std::move(wallet.GetOrchardSpendInfo(orchardEntries)[0]); + auto builder3 = TransactionBuilder(consensusParams, 2, orchardTree.root()); + builder3.AddOrchardSpend(std::move(noteToSpend2.first), std::move(noteToSpend2.second)); + auto tx3 = builder3.Build().GetTxOrThrow(); + CWalletTx wtx3 {&wallet, tx3}; + + auto hash = wtx.GetHash(); + auto hash2 = wtx2.GetHash(); + auto hash3 = wtx3.GetHash(); + + // No conflicts for no spends (wtx is currently the only transaction in the wallet) + EXPECT_EQ(0, wallet.GetConflicts(hash).size()); + + // No conflicts for one spend + auto orchardTxMeta2 = wallet.GetOrchardWallet().AddNotesIfInvolvingMe(wtx2); + ASSERT_TRUE(orchardTxMeta2.has_value()); + EXPECT_FALSE(orchardTxMeta2.value().empty()); + wtx2.SetOrchardTxMeta(orchardTxMeta2.value()); + wallet.LoadWalletTx(wtx2); + EXPECT_EQ(0, wallet.GetConflicts(hash).size()); + + // Conflicts for two spends + auto orchardTxMeta3 = wallet.GetOrchardWallet().AddNotesIfInvolvingMe(wtx3); + ASSERT_TRUE(orchardTxMeta3.has_value()); + EXPECT_FALSE(orchardTxMeta3.value().empty()); + wtx3.SetOrchardTxMeta(orchardTxMeta3.value()); + wallet.LoadWalletTx(wtx3); + auto c3 = wallet.GetConflicts(hash); + EXPECT_EQ(2, c3.size()); + EXPECT_EQ(std::set({hash2, hash3}), c3); + + // Tear down + chainActive.SetTip(NULL); + mapBlockIndex.erase(blockHash); + + RegtestDeactivateNU5(); +} + TEST(WalletTests, SproutNullifierIsSpent) { SelectParams(CBaseChainParams::REGTEST); CWallet wallet(Params()); From 7b51afd84052d84dcac89c5d4ede019870ea9ae6 Mon Sep 17 00:00:00 2001 From: therealyingtong Date: Fri, 25 Mar 2022 09:36:07 +0800 Subject: [PATCH 2/5] Fix GetConflictedOrchardNotes test. --- src/wallet/gtest/test_wallet.cpp | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/wallet/gtest/test_wallet.cpp b/src/wallet/gtest/test_wallet.cpp index 2e8a992d5..85d32136b 100644 --- a/src/wallet/gtest/test_wallet.cpp +++ b/src/wallet/gtest/test_wallet.cpp @@ -866,7 +866,6 @@ TEST(WalletTests, GetConflictedOrchardNotes) { // Simulate receiving new block and ChainTip signal wallet.IncrementNoteWitnesses(Params().GetConsensus(),&fakeIndex, &block, sproutTree, saplingTree, true); - wallet.UpdateSaplingNullifierNoteMapForBlock(&block); // Fetch the Orchard note so we can spend it. std::vector sproutEntries; @@ -895,12 +894,12 @@ TEST(WalletTests, GetConflictedOrchardNotes) { auto tx3 = builder3.Build().GetTxOrThrow(); CWalletTx wtx3 {&wallet, tx3}; - auto hash = wtx.GetHash(); auto hash2 = wtx2.GetHash(); auto hash3 = wtx3.GetHash(); // No conflicts for no spends (wtx is currently the only transaction in the wallet) - EXPECT_EQ(0, wallet.GetConflicts(hash).size()); + EXPECT_EQ(0, wallet.GetConflicts(hash2).size()); + EXPECT_EQ(0, wallet.GetConflicts(hash3).size()); // No conflicts for one spend auto orchardTxMeta2 = wallet.GetOrchardWallet().AddNotesIfInvolvingMe(wtx2); @@ -908,7 +907,8 @@ TEST(WalletTests, GetConflictedOrchardNotes) { EXPECT_FALSE(orchardTxMeta2.value().empty()); wtx2.SetOrchardTxMeta(orchardTxMeta2.value()); wallet.LoadWalletTx(wtx2); - EXPECT_EQ(0, wallet.GetConflicts(hash).size()); + EXPECT_EQ(0, wallet.GetConflicts(hash2).size()); + EXPECT_EQ(0, wallet.GetConflicts(hash3).size()); // Conflicts for two spends auto orchardTxMeta3 = wallet.GetOrchardWallet().AddNotesIfInvolvingMe(wtx3); @@ -916,7 +916,7 @@ TEST(WalletTests, GetConflictedOrchardNotes) { EXPECT_FALSE(orchardTxMeta3.value().empty()); wtx3.SetOrchardTxMeta(orchardTxMeta3.value()); wallet.LoadWalletTx(wtx3); - auto c3 = wallet.GetConflicts(hash); + auto c3 = wallet.GetConflicts(hash2); EXPECT_EQ(2, c3.size()); EXPECT_EQ(std::set({hash2, hash3}), c3); From 637592ebd58561dce996bf555faffde2a503c4e6 Mon Sep 17 00:00:00 2001 From: therealyingtong Date: Fri, 25 Mar 2022 10:37:17 +0800 Subject: [PATCH 3/5] Introduce OrchardWallet::GetPotentialSpendsFromNullifier method. --- src/rust/include/rust/orchard/wallet.h | 12 ++++++++++++ src/rust/src/wallet.rs | 18 ++++++++++++++++++ src/wallet/orchard.h | 11 +++++++++++ src/wallet/wallet.cpp | 5 ++--- 4 files changed, 43 insertions(+), 3 deletions(-) diff --git a/src/rust/include/rust/orchard/wallet.h b/src/rust/include/rust/orchard/wallet.h index 2c58b9206..fd787c054 100644 --- a/src/rust/include/rust/orchard/wallet.h +++ b/src/rust/include/rust/orchard/wallet.h @@ -333,6 +333,18 @@ void orchard_wallet_get_potential_spends( push_txid_callback_t push_cb ); +/** + * Returns a vector of transaction IDs for transactions that have been observed as + * spending the given nullifier by using the `push_cb` callback to push transaction + * IDs onto the provided result vector. + */ +void orchard_wallet_get_potential_spends_from_nullifier( + const OrchardWalletPtr* wallet, + const uint256& nullifier, + void* resultVector, + push_txid_callback_t push_cb + ); + /** * Fetches the information needed to spend the wallet note at the given outpoint, * relative to the current root known to the wallet of the Orchard commitment diff --git a/src/rust/src/wallet.rs b/src/rust/src/wallet.rs index 6d4670f47..28008e21b 100644 --- a/src/rust/src/wallet.rs +++ b/src/rust/src/wallet.rs @@ -1125,6 +1125,24 @@ pub extern "C" fn orchard_wallet_get_potential_spends( } } +#[no_mangle] +pub extern "C" fn orchard_wallet_get_potential_spends_from_nullifier( + wallet: *const Wallet, + nullifier: *const [c_uchar; 32], + result: Option, + push_cb: Option, +) { + let wallet = unsafe { wallet.as_ref() }.expect("Wallet pointer may not be null."); + let nullifier = + Nullifier::from_bytes(&*unsafe { nullifier.as_ref() }.expect("nullifier may not be null.")); + + if let Some(inpoints) = wallet.potential_spends.get(&nullifier.unwrap()) { + for inpoint in inpoints { + unsafe { (push_cb.unwrap())(result, inpoint.txid.as_ref()) }; + } + } +} + #[no_mangle] pub extern "C" fn orchard_wallet_get_spend_info( wallet: *const Wallet, diff --git a/src/wallet/orchard.h b/src/wallet/orchard.h index 894c21362..78c4cdb96 100644 --- a/src/wallet/orchard.h +++ b/src/wallet/orchard.h @@ -410,6 +410,17 @@ public: reinterpret_cast*>(txidsRet)->push_back(txid_out); } + std::vector GetPotentialSpendsFromNullifier(const uint256& nullifier) const { + std::vector result; + orchard_wallet_get_potential_spends_from_nullifier( + inner.get(), + nullifier, + &result, + PushTxId + ); + return result; + } + std::vector GetPotentialSpends(const OrchardOutPoint& outPoint) const { std::vector result; orchard_wallet_get_potential_spends( diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 4809189a1..2128f1f45 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -1668,9 +1668,8 @@ set CWallet::GetConflicts(const uint256& txid) const } } - for (uint32_t i = 0; i < wtx.GetOrchardBundle().GetNumActions(); i++) { - OrchardOutPoint op(wtx.GetHash(), i); - auto potential_spends = orchardWallet.GetPotentialSpends(op); + for (const uint256& nullifier : wtx.GetOrchardBundle().GetNullifiers()) { + auto potential_spends = orchardWallet.GetPotentialSpendsFromNullifier(nullifier); if (potential_spends.size() <= 1) { continue; // No conflict if zero or one spends From 2ef41824ff737fb2a48a192287ec2af8f06f8759 Mon Sep 17 00:00:00 2001 From: ying tong Date: Fri, 25 Mar 2022 12:07:46 +0800 Subject: [PATCH 4/5] orchard_wallet_get_potential_spends: Do not use uint256 type. We don't want to depend on the C++ uint256 type in the C FFI. Co-authored-by: str4d --- src/rust/include/rust/orchard/wallet.h | 2 +- src/wallet/orchard.h | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/rust/include/rust/orchard/wallet.h b/src/rust/include/rust/orchard/wallet.h index fd787c054..dcf65c5e1 100644 --- a/src/rust/include/rust/orchard/wallet.h +++ b/src/rust/include/rust/orchard/wallet.h @@ -340,7 +340,7 @@ void orchard_wallet_get_potential_spends( */ void orchard_wallet_get_potential_spends_from_nullifier( const OrchardWalletPtr* wallet, - const uint256& nullifier, + const unsigned char *nullifier, void* resultVector, push_txid_callback_t push_cb ); diff --git a/src/wallet/orchard.h b/src/wallet/orchard.h index 78c4cdb96..45e5a3ab4 100644 --- a/src/wallet/orchard.h +++ b/src/wallet/orchard.h @@ -414,7 +414,7 @@ public: std::vector result; orchard_wallet_get_potential_spends_from_nullifier( inner.get(), - nullifier, + nullifier.begin(), &result, PushTxId ); From 74f770426b292694533350cd0d0fd325194b8fa9 Mon Sep 17 00:00:00 2001 From: Kris Nuttycombe Date: Fri, 25 Mar 2022 18:37:31 -0600 Subject: [PATCH 5/5] Apply suggestions from code review --- src/rust/src/wallet.rs | 2 +- src/wallet/gtest/test_wallet.cpp | 1 - 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/src/rust/src/wallet.rs b/src/rust/src/wallet.rs index 28008e21b..8623a8b26 100644 --- a/src/rust/src/wallet.rs +++ b/src/rust/src/wallet.rs @@ -1134,7 +1134,7 @@ pub extern "C" fn orchard_wallet_get_potential_spends_from_nullifier( ) { let wallet = unsafe { wallet.as_ref() }.expect("Wallet pointer may not be null."); let nullifier = - Nullifier::from_bytes(&*unsafe { nullifier.as_ref() }.expect("nullifier may not be null.")); + Nullifier::from_bytes(unsafe { nullifier.as_ref() }.expect("nullifier may not be null.")); if let Some(inpoints) = wallet.potential_spends.get(&nullifier.unwrap()) { for inpoint in inpoints { diff --git a/src/wallet/gtest/test_wallet.cpp b/src/wallet/gtest/test_wallet.cpp index 85d32136b..d12e77595 100644 --- a/src/wallet/gtest/test_wallet.cpp +++ b/src/wallet/gtest/test_wallet.cpp @@ -823,7 +823,6 @@ TEST(WalletTests, GetConflictedOrchardNotes) { auto recipient = ivk.Address(j); uint256 orchardAnchor; - orchard::Builder(true, true, orchardAnchor); // Generate transparent funds CBasicKeyStore keystore;