Skip to content

Commit 74a1a5d

Browse files
committed
wallet: Check spent Orchard notes in CWallet{Tx}::IsFromMe
We missed these methods when integrating Orchard into the wallet. - We didn't notice `CWallet::IsFromMe` because while previous shielded protocols did spend detection in `CWallet::AddToWalletIfInvolvingMe` via that logic, the Orchard logic was implemented in a separate internal Rust wallet with its own detection logic. Technically the change is unnecessary here as `CWallet::AddToWalletIfInvolvingMe` is the only user of this method, but we update it for consistency. - We didn't notice `CWalletTx::IsFromMe` because the only critical place it affects is the `getbalance` RPC (other than that, it is only called from code used to either create purely-transparent transactions, or provide informational output on non-critical RPC methods). We had a similar bug with this method previously where it didn't track shielded at all (zcash/zcash@fd49f78).
1 parent 8fa26d7 commit 74a1a5d

File tree

5 files changed

+48
-0
lines changed

5 files changed

+48
-0
lines changed

doc/release-notes.md

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,3 +4,18 @@ release-notes at release time)
44
Notable changes
55
===============
66

7+
Fixed Orchard bug in transparent balance APIs
8+
---------------------------------------------
9+
10+
Several RPC methods inherited from Bitcoin Core internally rely on
11+
`CWalletTx::IsFromMe` for detecting involvement of the wallet in the input
12+
side of a transaction. For example:
13+
- The `getbalance` RPC method uses it as part of identifying "trusted"
14+
zero-confirmation transactions to include in the balance calculation.
15+
- The `gettransaction` RPC method uses it to decide whether to include a
16+
`fee` field in its response.
17+
18+
When Orchard was integrated into `zcashd`, this method was not updated to
19+
account for it, meaning that unshielding transactions spending Orchard notes
20+
would not be correctly accounted for in transparent-specific RPC methods. A
21+
similar bug involving Sprout and Sapling spends was fixed in v4.5.1.

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

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -335,6 +335,13 @@ bool orchard_wallet_get_txdata(
335335
push_output_t push_output_cb
336336
);
337337

338+
/**
339+
* Returns true if this nullifier belongs to one of the notes in our wallet.
340+
*/
341+
bool orchard_wallet_is_nullifier_from_me(
342+
const OrchardWalletPtr* wallet,
343+
const unsigned char *nullifier);
344+
338345
typedef void (*push_txid_callback_t)(void* resultVector, unsigned char txid[32]);
339346

340347
/**

src/rust/src/wallet.rs

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1183,6 +1183,18 @@ pub extern "C" fn orchard_wallet_get_txdata(
11831183
}
11841184
}
11851185

1186+
#[no_mangle]
1187+
pub extern "C" fn orchard_wallet_is_nullifier_from_me(
1188+
wallet: *const Wallet,
1189+
nullifier: *const [c_uchar; 32],
1190+
) -> bool {
1191+
let wallet = unsafe { wallet.as_ref() }.expect("Wallet pointer may not be null.");
1192+
let nullifier =
1193+
Nullifier::from_bytes(unsafe { nullifier.as_ref() }.expect("nullifier may not be null."));
1194+
1195+
wallet.nullifiers.contains_key(&nullifier.unwrap())
1196+
}
1197+
11861198
pub type PushTxId = unsafe extern "C" fn(obj: Option<FFICallbackReceiver>, txid: *const [u8; 32]);
11871199

11881200
#[no_mangle]

src/wallet/orchard.h

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -437,6 +437,10 @@ class OrchardWallet
437437
reinterpret_cast<std::vector<uint256>*>(txidsRet)->push_back(txid_out);
438438
}
439439

440+
bool IsNullifierFromMe(const std::array<uint8_t, 32>& nullifier) const {
441+
return orchard_wallet_is_nullifier_from_me(inner.get(), nullifier.data());
442+
}
443+
440444
std::vector<uint256> GetPotentialSpendsFromNullifier(const uint256& nullifier) const {
441445
std::vector<uint256> result;
442446
orchard_wallet_get_potential_spends_from_nullifier(

src/wallet/wallet.cpp

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4082,6 +4082,11 @@ bool CWallet::IsFromMe(const CTransaction& tx) const
40824082
return true;
40834083
}
40844084
}
4085+
for (const auto& action : tx.GetOrchardBundle().GetDetails()->actions()) {
4086+
if (orchardWallet.IsNullifierFromMe(action.nullifier())) {
4087+
return true;
4088+
}
4089+
}
40854090
return false;
40864091
}
40874092

@@ -4981,6 +4986,11 @@ bool CWalletTx::IsFromMe(const isminefilter& filter) const
49814986
return true;
49824987
}
49834988
}
4989+
for (const auto& action : GetOrchardBundle().GetDetails()->actions()) {
4990+
if (pwallet->orchardWallet.IsNullifierFromMe(action.nullifier())) {
4991+
return true;
4992+
}
4993+
}
49844994
return false;
49854995
}
49864996

0 commit comments

Comments
 (0)