Add queues to track the need for transaction enhancement and/or verification of mined status.

[nuttycom]

No description provided.

[codecov]

Codecov Report

Attention: Patch coverage is 79.27461% with 40 lines in your changes missing coverage. Please review.

Project coverage is 61.16%. Comparing base (7f7b685) to head (5a1645a).
Report is 8 commits behind head on main.

Files	Patch %	Lines
zcash_client_sqlite/src/wallet.rs	83.07%	11 Missing ⚠️
zcash_client_sqlite/src/wallet/transparent.rs	73.80%	11 Missing ⚠️
zcash_client_sqlite/src/lib.rs	86.00%	7 Missing ⚠️
zcash_client_backend/src/data_api.rs	28.57%	5 Missing ⚠️
...e/src/wallet/init/migrations/tx_retrieval_queue.rs	66.66%	4 Missing ⚠️
zcash_client_sqlite/src/wallet/orchard.rs	0.00%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1473      +/-   ##
==========================================
+ Coverage   60.82%   61.16%   +0.34%     
==========================================
  Files         140      141       +1     
  Lines       16472    16650     +178     
==========================================
+ Hits        10019    10184     +165     
- Misses       6453     6466      +13     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[str4d]

Flushing comments up to 40d94d0 (just before looking at db.rs).

[str4d]

Reviewed 1cbced3.

[str4d]

utACK 171cc3d once my concern about Enhancement results has been addressed (either by explaining why it's fine, or making a fix).

[str4d]

This is true for Status requests, but not true for Enhancement requests which subsume Status.

[daira]

Yes. I noticed that and the first commit of nuttycom#39 (nuttycom@239f7ff) fixes it. The implementation is fine, it's just the comment that is incorrect.

[nuttycom]

It occurs to me that https://github.com/zcash/librustzcash/pull/1473/files#diff-a1724f7547ecabc613a07520eaf7917d9084d4277219d186698b0c8e352495a4R2425-R2427 is superfluous; we should never have a Status request for a transaction unless we have the raw data. So one way to fix this would be to remove the query type argument from the queue_tx_retrieval function, and to set it based upon the presence or absence of raw transaction data.

[str4d]

The comment change doesn't resolve my concern. GetStatus is indeed only generated when we have the raw transaction (as I noted), but Enhancement is generated by definition when we don't have the raw transaction, and its documentation states that when the caller is unable to look up the transaction, it must call this method.

The reason it is okay to delete for Enhancement requests is that:

• If a transaction can't be enhanced, we assume that the caller calls this method with TransactionStatus::TxidNotRecognized (as the other two statuses mean that the chain data source knew about the transaction, and therefore must have observed it and could return its raw data), so we don't need to consider what happens in the other statuses.
• We only delete for TransactionStatus::TxidNotRecognized if the transaction is expired, at which point we don't necessarily care about enhancement (though the user might still care about the memo), and it is very likely that we will be unable to ever enhance (as the tx will have been dropped from all mempools).

[nuttycom]

Simplified in 5a1645a

[nuttycom]

The fact is that an Enhancement request is a Status request. That's what it means for an Enhancement request to subsume a Status request. It's either "get me the status" or "get me the status and the data if it's available".

[str4d]

@daira Enhancement is getting the status; it subsumes / is a superset of GetStatus. That's why I raised my initial concern that this method was being written ostensibly assuming it would only be called via GetStatus, which was false.

As @nuttycom says, the actual implementation does correctly work with both GetStatus and Enhancement; the comment was just misleading.

[nuttycom]

I have now updated this comment again. (b47c7bf)

[daira]

The fact is that an Enhancement request is a Status request.

Not so. Otherwise it wouldn't have to requeue a Status request when queue_status_retrieval is true. It doesn't directly update the mined height in the case where d_tx.mined_height().is_none().

[nuttycom]

Otherwise it wouldn't have to requeue a Status request when queue_status_retrieval is true.

The re-queueing is so that the wallet continues to monitor for when the transaction is eventually mined. A relevant case here is that store_decrypted_tx was called with a transaction that was read from the mempool; we have complete transaction data, but we don't know yet whether the transaction will be mined, so we stick the status request back on the queue. For a fully transparent transaction that we get from the mempool we have no other way of detecting that it becomes mined.

[daira]

utACK modulo suggested changes in nuttycom#39 . We can either merge that PR into this one, or merge a copy of it separately. (I have checked that it passes tests locally.)

[str4d]

utACK b47c7bf

[daira]

Post-hoc ACK