`zcash_client_*`: Fix bug where transparent funds can't be shielded before the normal confirmation depth

[str4d]

We've had Zashi user reports that they can't shield funds after 1 confirmation as they expect, and instead have to wait for 10 confirmations (used for normal spending). The mobile SDKs don't allow shielding funds if there are insufficient funds to shield in the wallet balance.

[str4d]

I think I found the cause of the bug in zcash_client_sqlite here: https://github.com/zcash/librustzcash/pull/1257#discussion_r1637994605

https://github.com/zcash/librustzcash/blob/5a2a670ffca5d81e54c166d0f6b5cf5bcce84f32/zcash_client_sqlite/src/wallet.rs#L1299-L1305

The :max_height variable is set to zero_conf_height, but the latter is not actually zero-conf, it's the same as summary_height (except that it doesn't force min_confirmations to be at least 1).

[str4d]

I think we have fixed this; we should check.

[daira]

@true-jared wrote in #1572 which I'm merging into this issue:

Even though the [iOS] SDK has 1 confirmation set let minimumShieldingConfirmations: UInt32 = 1 we still need to wait 10 confirmations.

How to reproduce:

• I tapped a button to shield my funds, it:
• created a transaction in a pending state with is_shielding TRUE
• after 1st confirmation, it flipped to sending instead of shielding, I checked the DB and there is no TRUE anymore (see the DB)
• after 10 confirmations, it flipped to sent
• when I restore the same wallet, I see shielded again

Few more observations:

• when I try to restore the wallet on a different simulator while it’s still waiting on 10 confirmations and should be shielding funds but it’s sending, I see correct state restored.. so restore is able to bring the TRUE flag back but where it matters it’s broken

• db export shared via Slack here: https://zcash.slack.com/archives/C02C1GZ0R6F/p1724835111523729

This was filed 2 weeks ago (October 14th) which suggests that it is not fixed; however, we need to check whether all of the changes @str4d was thinking of got into the (iOS) SDK version that was being used for this report.

[daira]

Copying a comment I wrote at https://github.com/zcash/librustzcash/pull/1570/files#r1817869234 here. Note that it mainly concerns the fact that we should not expect to be able to do zero-conf shielding yet, but the information about where the number of confirmations is being set in each SDK may be useful.

Yes, min_confirmations is not being used at all for transparent TXOs, and that's a bug that this PR [#1570] would fix. But @nuttycom is incorrect (here) in asserting that "we allow shielding at minconf 0 [in the mobile wallets]". We do not, for at least two reasons on each platform:

1. (Swift/iOS) The SDK defines minimumShieldingConfirmations as 1. It uses that both in the call to zcashlc_get_verified_transparent_balance_for_account to get the shieldable balance, and in the call to zcashlc_propose_shielding.

   (Android) The SDK sets min_confirmations to 1 when determining the target height for the call to get_spendable_transparent_outputs in the JNI code for RustBackend.getTotalTransparentBalance. Similarly min_confirmations_for_height is 1 when deciding whether there are any shieldable funds in RustBackend.proposeShielding. This is independent of the fact that min_confirmations is 0 in the call to propose_shielding.

2. None of the balances returned from get_wallet_summary include unmined UTXOs. That also applies to unshielded_balance before this PR. The reason for transparent balances is this WHERE clause condition (line 395 before / line 397 after #1570):

       t.mined_height < :mempool_height -- tx is mined
   

   And so we do not see unmined transparent UTXOs as available to shield, even if min_confirmations were to be passed as 0 to get_wallet_summary.

If we want to allow zero-conf shielding then we should do so separately. This PR should IMHO preserve the current behaviour of treating min_confirmations == 0 the same as min_confirmations == 1 for both shielded and unshielded balances (but explicitly document it, as I suggested here). The bugfix of also applying min_confirmations to unshielded balances would then only affect min_confirmations > 1, for now.

Zero-conf shielding is entirely untested and, if we decide to enable that, we need to take more time over it to avoid introducing bugs.

[nuttycom]

I just successfully performed 1-conf shielding with Zashi 1.2.2 (which updates to the latest SDK release). So I'm closing this as fixed.