zcash_client_sqlite: Track all transparent spends. #1496
Conversation
Prior to this change, the `mark_transparent_utxo_spent` method assumed that the UTXO information for outputs belonging to the wallet would be known to exist before their spends could be detected. However, this is not true in transparent history recovery: the spends are detected first. We now cache the information about those spends so that we can then correctly record the spend when the output being spent is eventually detected. At present, data from this spend cache is never deleted. This is because such deletions could undermine history recovery in some narrow cases related to chain reorgs.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1496 +/- ##
==========================================
+ Coverage 61.12% 61.14% +0.01%
==========================================
Files 141 141
Lines 16650 16658 +8
==========================================
+ Hits 10178 10185 +7
- Misses 6472 6473 +1 ☔ View full report in Codecov by Sentry. |
…actions in `store_decrypted_tx`
| wallet::store_decrypted_tx( | ||
| transaction, | ||
| &self.params, | ||
| DecryptedTransaction::new(mined_height, &tx, vec![], vec![]), | ||
| )?; |
There was a problem hiding this comment.
Hmm. This is a very complicated function to be calling in a migration. If we need to change it, will we duplicate it like we do for code called in other migrations?
There was a problem hiding this comment.
What I would like to do with store_decrypted_tx in general is to refactor it to abstract over the database operations (i.e. factor out the business logic) and perhaps even move the main logic to zcash_client_backend (relying upon some lower-level data access API trait.) That will make it easier to modularize it in a way that makes the migration maintenance less annoying.
This permits this method to be used in a migration.
nuttycom
force-pushed
the
fix/transparent_spend_recording
branch
from
25b9ccb
to
1e60482
Compare
…ore additional transparent history.
…quests do not exceed the chain tip. `lightwalletd` will return an error in the case that the requested end height exceeds the chain tip. This should be considered a bug in lightwalletd, but for now we will work around it in the wallet.
nuttycom
force-pushed
the
fix/transparent_spend_recording
branch
from
1e60482
to
5a32d3b
Compare
| // `lightwalletd` will return an error for `GetTaddressTxids` requests having an end height | ||
| // greater than the current chain tip height, so we take the chain tip height into account | ||
| // here in order to make this pothole easier for clients of the API to avoid. | ||
| let chain_tip_height = super::chain_tip_height(conn)?; |
There was a problem hiding this comment.
Incidentally, why does the doc for wallet::chain_tip_height say
/// Returns the minimum and maximum heights of blocks in the chain which may be scanned.when it actually only returns (optionally) a single height?
Prior to this change, the
mark_transparent_utxo_spentmethod assumed that the UTXO information for outputs belonging to the wallet would be known to exist before their spends could be detected. However, this is not true in transparent history recovery: the spends are detected first. We now cache the information about those spends so that we can then correctly record the spend when the output being spent is eventually detected.At present, data from this spend cache is never deleted. This is because such deletions could undermine history recovery in some narrow cases related to chain reorgs.