Merge pull request zcash#7035 from str4d/fix-wtx-is-from-me-orchard

wallet: Check spent Orchard notes in `CWallet{Tx}::IsFromMe`

Author: nuttycom

doc/release-notes.md

@@ -4,3 +4,18 @@ release-notes at release time)
 Notable changes
 ===============
 
+Fixed Orchard bug in transparent balance APIs
+---------------------------------------------
+
+Several RPC methods inherited from Bitcoin Core internally rely on
+`CWalletTx::IsFromMe` for detecting involvement of the wallet in the input
+side of a transaction. For example:
+- The `getbalance` RPC method uses it as part of identifying "trusted"
+  zero-confirmation transactions to include in the balance calculation.
+- The `gettransaction` RPC method uses it to decide whether to include a
+  `fee` field in its response.
+
+When Orchard was integrated into `zcashd`, this method was not updated to
+account for it, meaning that unshielding transactions spending Orchard notes
+would not be correctly accounted for in transparent-specific RPC methods. A
+similar bug involving Sprout and Sapling spends was fixed in v4.5.1.

qa/rpc-tests/wallet_isfromme.py

@@ -1,5 +1,5 @@
 #!/usr/bin/env python3
-# Copyright (c) 2021-2024 The Zcash developers
+# Copyright (c) 2021-2025 The Zcash developers
 # Distributed under the MIT software license, see the accompanying
 # file COPYING or https://www.opensource.org/licenses/mit-license.php .
 
@@ -8,6 +8,7 @@
     BLOSSOM_BRANCH_ID,
     CANOPY_BRANCH_ID,
     HEARTWOOD_BRANCH_ID,
+    NU5_BRANCH_ID,
     OVERWINTER_BRANCH_ID,
     SAPLING_BRANCH_ID,
     assert_equal,
@@ -32,6 +33,7 @@ def setup_network(self, split=False):
             nuparams(BLOSSOM_BRANCH_ID, 1),
             nuparams(HEARTWOOD_BRANCH_ID, 1),
             nuparams(CANOPY_BRANCH_ID, 1),
+            nuparams(NU5_BRANCH_ID, 110),
             '-allowdeprecated=getnewaddress',
             '-allowdeprecated=z_getnewaddress',
         ]])
@@ -85,5 +87,51 @@ def run_test (self):
         # "getbalance '' 0" should count both outputs. The bug failed here.
         assert_equal(Decimal(node.getbalance('', 0)), Decimal('12.5') - coinbase_fee - fee)
 
+        # Mine the transaction; we get another coinbase output.
+        node.generate(8)
+        self.sync_all()
+        assert_equal(Decimal(node.getbalance('', 0)), Decimal('62.5') - coinbase_fee - fee)
+
+        # Send all available funds to an Orchard UA.
+        acct = node.z_getnewaccount()['account']
+        orchard_ua = node.z_getaddressforaccount(acct, ['orchard'])['address']
+        recipients = [{'address': orchard_ua, 'amount': Decimal('6.25') - coinbase_fee - 2*fee}]
+        # minconf = 1 is requires for `TransactionEffects::ApproveAndBuild`
+        # to use an Orchard anchor.
+        opid = node.z_sendmany(taddr, recipients, 1, fee, 'AllowRevealedSenders')
+        wait_and_assert_operationid_status(node, opid)
+        self.sync_all()
+        assert_equal(Decimal(node.getbalance('', 0)), Decimal('62.5') - Decimal('6.25'))
+
+        # Mine the transaction; we get another coinbase output.
+        node.generate(1)
+        self.sync_all()
+        assert_equal(Decimal(node.getbalance('', 0)), Decimal('68.75') - Decimal('6.25'))
+
+        # Now send the funds back to a new t-address.
+        taddr = node.getnewaddress()
+        recipients = [{'address': taddr, 'amount': Decimal('6.25') - coinbase_fee - 3*fee}]
+        # TODO: this fails for ZIP_317_FEE due to a dust threshold problem:
+        # "Insufficient funds: have 6.24985, need 0.00000054 more to surpass the dust
+        # threshold and avoid being forced to over-pay the fee. Alternatively, you could
+        # specify a fee of 0.0001 to allow overpayment of the conventional fee and have
+        # this transaction proceed.; note that coinbase outputs will not be selected if
+        # you specify ANY_TADDR, any transparent recipients are included, or if the
+        # `privacyPolicy` parameter is not set to `AllowRevealedSenders` or weaker."
+        opid = node.z_sendmany(orchard_ua, recipients, 1, fee, 'AllowRevealedRecipients')
+        wait_and_assert_operationid_status(node, opid)
+        self.sync_all()
+
+        # At this point we have created the conditions for the bug fixed in
+        # https://github.com/zcash/zcash/pull/5325.
+
+        # listunspent should show the coinbase outputs, and optionally the
+        # newly-received unshielding output.
+        assert_equal(len(node.listunspent()), 10)
+        assert_equal(len(node.listunspent(0)), 11)
+
+        # "getbalance '' 0" should count both outputs. The Orchard bug failed here.
+        assert_equal(Decimal(node.getbalance('', 0)), Decimal('68.75') - coinbase_fee - 3*fee)
+
 if __name__ == '__main__':
     WalletIsFromMe().main ()

src/rust/include/rust/orchard/wallet.h

@@ -335,6 +335,13 @@ bool orchard_wallet_get_txdata(
         push_output_t push_output_cb
         );
 
+/**
+ * Returns true if this nullifier belongs to one of the notes in our wallet.
+ */
+bool orchard_wallet_is_nullifier_from_me(
+        const OrchardWalletPtr* wallet,
+        const unsigned char *nullifier);
+
 typedef void (*push_txid_callback_t)(void* resultVector, unsigned char txid[32]);
 
 /**

src/rust/src/wallet.rs

@@ -1183,6 +1183,18 @@ pub extern "C" fn orchard_wallet_get_txdata(
     }
 }
 
+#[no_mangle]
+pub extern "C" fn orchard_wallet_is_nullifier_from_me(
+    wallet: *const Wallet,
+    nullifier: *const [c_uchar; 32],
+) -> bool {
+    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."));
+
+    wallet.nullifiers.contains_key(&nullifier.unwrap())
+}
+
 pub type PushTxId = unsafe extern "C" fn(obj: Option<FFICallbackReceiver>, txid: *const [u8; 32]);
 
 #[no_mangle]

src/wallet/orchard.h

@@ -437,6 +437,10 @@ class OrchardWallet
         reinterpret_cast<std::vector<uint256>*>(txidsRet)->push_back(txid_out);
     }
 
+    bool IsNullifierFromMe(const std::array<uint8_t, 32>& nullifier) const {
+        return orchard_wallet_is_nullifier_from_me(inner.get(), nullifier.data());
+    }
+
     std::vector<uint256> GetPotentialSpendsFromNullifier(const uint256& nullifier) const {
         std::vector<uint256> result;
         orchard_wallet_get_potential_spends_from_nullifier(

src/wallet/wallet.cpp

@@ -4082,6 +4082,11 @@ bool CWallet::IsFromMe(const CTransaction& tx) const
             return true;
         }
     }
+    for (const auto& action : tx.GetOrchardBundle().GetDetails()->actions()) {
+        if (orchardWallet.IsNullifierFromMe(action.nullifier())) {
+            return true;
+        }
+    }
     return false;
 }
 
@@ -4981,6 +4986,11 @@ bool CWalletTx::IsFromMe(const isminefilter& filter) const
             return true;
         }
     }
+    for (const auto& action : GetOrchardBundle().GetDetails()->actions()) {
+        if (pwallet->orchardWallet.IsNullifierFromMe(action.nullifier())) {
+            return true;
+        }
+    }
     return false;
 }