Bug in is_shielding state going from TRUE to FALSE after the first confirmation

[true-jared]

• the states of the DB and is_shielding changes from true to false

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

[nuttycom]

The root of the problem here appears to be that the received note in the shielding transaction is no longer interpreted as a change output after the transaction is mined.

This is the code that is causing the transaction to be marked as not shielding:

librustzcash/zcash_client_sqlite/src/wallet/db.rs

Lines 854 to 855 in bf8b39a

	-- We do not know about any external outputs of the transaction.
	AND MAX(COALESCE(sent_note_counts.sent_notes, 0)) = 0

In investigating the source of the sent_note_counts data, and comparing the two provided database files, I found that the value of the is_change column in the orchard_received_notes table is changing from 1 to 0 at the point that the transaction is discovered via chain scanning.

[nuttycom]

Noticed along the way: the following lines don't make sense, as the is_change field is not optional, so the nullability check will never return false:

• librustzcash/zcash_client_sqlite/src/wallet/orchard.rs

  Line 255 in bf8b39a

  is_change = IFNULL(:is_change, is_change),

• librustzcash/zcash_client_sqlite/src/wallet/sapling.rs

  Line 358 in bf8b39a

  is_change = IFNULL(:is_change, is_change),

This could usefully be implemented as MAX(:is_change, is_change) instead and would work around this bug, but the actual root cause is somewhere upstream of this, in the is_change determination coming from scanning.

[nuttycom]

The root cause of the is_change field being set to false in shielding transactions is that

librustzcash/zcash_client_backend/src/scanning.rs

Lines 822 to 849 in bf8b39a

	let (sapling_spends, sapling_unlinked_nullifiers) = find_spent(
	&tx.spends,
	&nullifiers.sapling,
	|spend| {
	spend.nf().expect(
	"Could not deserialize nullifier for spend from protobuf representation.",
	)
	},
	WalletSpend::from_parts,
	);
	sapling_nullifier_map.push((txid, tx_index, sapling_unlinked_nullifiers));
	#[cfg(feature = "orchard")]
	let orchard_spends = {
	let (orchard_spends, orchard_unlinked_nullifiers) = find_spent(
	&tx.actions,
	&nullifiers.orchard,
	|spend| {
	spend.nf().expect(
	"Could not deserialize nullifier for spend from protobuf representation.",
	)
	},
	WalletSpend::from_parts,
	);
	orchard_nullifier_map.push((txid, tx_index, orchard_unlinked_nullifiers));
	orchard_spends
	};

does not (and cannot) determine that the spend is from the same account, because the spent UTXO is transparent.

To fix this, we could query the backend to augment the spent_from_account set for the transaction. This would work for the particular case of shielding transactions in the wallet steady state, because the backend will already have the transaction data. However, this may not be satisfactory in e.g. wallet recovery cases, because the shielded parts of the compact block could conceivably be scanned before the transparent UTXO DAG is traversed. Therefore, we may need to make changes in other places to ensure that stored notes are updated to correct their is_change values if we decide to take this route. It would also require a database capability to be added in a place that it doesn't currently exist, so this seems like a less desirable approach.

Remediation:

• We should make the MAX change previously described; that is noninvasive and obviously correct.
• The existing code doesn't appear to be taking the key scope into account for the is_change determination. This was an intentional choice though, because for example a cross-account wallet-internal transfer shouldn't be considered change.
• It's necessary to add a migration to fix incorrect is_change values in existing wallets for outputs of wallet-internal shielding transactions where the source account is the same as the recipient account.
• We should potentially change https://github.com/zcash/librustzcash/blob/main/zcash_client_sqlite/src/wallet/db.rs#L822 to use a comparison between source and recipient account instead of is_change? That might be complex to add to the query though.