Merge pull request #5732 from therealyingtong/test-conflicted-orchard-notes

Fix handling of Orchard notes in `CWallet::GetConflicts`.

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 unsigned char *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

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<FFICallbackReceiver>,
+    push_cb: Option<PushTxId>,
+) {
+    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,

src/wallet/gtest/test_wallet.cpp

@@ -806,6 +806,126 @@ 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;
+
+    // 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);
+
+    // Fetch the Orchard note so we can spend it.
+    std::vector<SproutNoteEntry> sproutEntries;
+    std::vector<SaplingNoteEntry> saplingEntries;
+    std::vector<OrchardNoteMetadata> 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 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(hash2).size());
+    EXPECT_EQ(0, wallet.GetConflicts(hash3).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(hash2).size());
+    EXPECT_EQ(0, wallet.GetConflicts(hash3).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(hash2);
+    EXPECT_EQ(2, c3.size());
+    EXPECT_EQ(std::set<uint256>({hash2, hash3}), c3);
+
+    // Tear down
+    chainActive.SetTip(NULL);
+    mapBlockIndex.erase(blockHash);
+
+    RegtestDeactivateNU5();
+}
+
 TEST(WalletTests, SproutNullifierIsSpent) {
     SelectParams(CBaseChainParams::REGTEST);
     CWallet wallet(Params());

src/wallet/orchard.h

@@ -410,6 +410,17 @@ public:
         reinterpret_cast<std::vector<uint256>*>(txidsRet)->push_back(txid_out);
     }
 
+    std::vector<uint256> GetPotentialSpendsFromNullifier(const uint256& nullifier) const {
+        std::vector<uint256> result;
+        orchard_wallet_get_potential_spends_from_nullifier(
+            inner.get(),
+            nullifier.begin(),
+            &result,
+            PushTxId
+            );
+        return result;
+    }
+
     std::vector<uint256> GetPotentialSpends(const OrchardOutPoint& outPoint) const {
         std::vector<uint256> result;
         orchard_wallet_get_potential_spends(

src/wallet/wallet.cpp

@@ -1694,9 +1694,8 @@ set<uint256> 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