zcash_client_sqlite: Fix transparent balance APIs
#985
Conversation
| // Unmine the shielding transaction via a reorg. | ||
| st.wallet_mut() | ||
| .truncate_to_height(mined_height - 1) | ||
| .unwrap(); |
There was a problem hiding this comment.
In writing the test and getting it to exhibit the intended behaviour, I noticed that after a truncation, WalletDb::chain_height returns mined_height + 9. This is due to the chain height being obtained from scan_queue, and WalletDb::truncate_to_height inserts a Verify range of length VERIFY_LOOKAHEAD just after the truncation height. This was somewhat unexpected to me, because we don't actually know that the current chain tip has reached that height (it could be a reorg to a shorter heavier chain, or it might be a reorg of only a few blocks), and WalletDb::chain_height is now used in several places as the source of truth for what was last passed into WalletDb::update_chain_tip.
There was a problem hiding this comment.
Hm, yeah, that Verify range insertion was from pretty early in my implementation, and I don't think it necessarily makes sense any more. Instead, after the rewind the next update_chain_tip call should establish the scanning ranges.
There was a problem hiding this comment.
I think there is still a benefit to having a Verify range at the post-truncation tip, because the caller may not have rewound far enough, and if the new chain tip is scanned before we check the connection then the subsequent truncation will undo that new scan as well (causing more duplication of scanning work).
But it should be a verify range that doesn't extend past the truncated tip, which means we need at least one block of overlap (unlike the Verify range that update_chain_tip creates which sits adjacent to the prior tip, sufficient for connectivity checking but avoiding duplicate scanning). Probably just adding a 1-block range rescanning the truncated tip is sufficient; we already tell the caller to pick some rewind distance themselves, so there's no need to bake in additional duplicate scanning unless we want to change that interaction pattern (e.g. by having APIs that tell the caller what height to rewind to).
There was a problem hiding this comment.
What if the Verify range was just one block long?
There was a problem hiding this comment.
Hmm, yeah. It's probably reasonable to assume here that the caller rewound to a block before a problematic scan, which implies that there exists at least one block after the truncation height. So instead of my current fix (verify the truncation height), we could verify the block after the truncation height (but no further). Still slightly unintuitive in terms of the method name though, as chain_height would not match the truncation.
There was a problem hiding this comment.
I'm going to leave my current fix in place (rescan the truncated-to height, so the chain height doesn't differ from that height until update_chain_tip is called) unless review asks otherwise.
zcash_client_sqlite/src/wallet.rs
Outdated
| // Expire the shielding transaction. | ||
| let expiry_height = st.wallet().get_transaction(txid).unwrap().expiry_height(); | ||
| st.wallet_mut().update_chain_tip(expiry_height).unwrap(); | ||
|
|
||
| // The transparent output should be spendable again, with more confirmations. | ||
| check_balance(&st, 0, value); | ||
| check_balance(&st, 1, value); | ||
| check_balance(&st, 2, value); |
There was a problem hiding this comment.
I added this to the test because it occurred to me that this is something that can happen to user wallets, particularly for SDK 2.0.0 where transactions will be using a fixed fee. However, this part of the test is failing because we don't have any handling for this situation. We need to decide whether we want to handle this.
There was a problem hiding this comment.
Let's talk tomorrow to figure out where it makes sense to handle this. Up until now transaction expiry has been treated as sort of a passive thing. I think that the correct solution here is going to be for the queries that are computing transparent balance to ensure that the spending transactions have not expired unmined. So, a UTXO might have a "spent in tx" field that is non-null, but that UTXO should still be treated as spendable if the referenced transaction has expired unmined, and future spends of that UTXO should then overwrite the spent-in-tx field.
There was a problem hiding this comment.
I suspect this should be a post-2.0 cleanup.
str4d
force-pushed
the
983-transparent-balance-fix
branch
from
f165611 to
5aa1e6b
Compare
str4d
force-pushed
the
983-transparent-balance-fix
branch
from
60063a4 to
6a9ad39
Compare
| @@ -490,7 +490,7 @@ impl<Nf> ScannedBlock<Nf> { | |||
| } | |||
|
|
|||
| /// Returns the metadata describing the state of the note commitment trees as of the end of the | |||
| /// scanned block. | |||
| /// scanned block. | |||
The `LEFT OUTER JOIN` was causing the `tx.block IS NULL` check to alias two cases: an unspent transparent output, and a transparent output spent in an unmined transaction. The latter only makes sense to include in the UTXO count if the transaction is expired, and (due to limitations of the transparent data model in the current wallet) if that expiry won't be undone by a reorg. We now handle these two cases directly. Partly reverts 8828276. Closes #983. Co-authored-by: Kris Nuttycombe <kris@nutty.land>
We don't know at truncation time what the latest chain tip is; the chain might have reorged to a shorter heavier chain, or the reorg depth might only be a few blocks. `WalletDb::chain_height` uses the scan queue as its source of truth, so the `Verify` range we add during truncation (to prioritise determining whether the rewind was sufficient) can't extend beyond the block height we know to exist. The next call to `WalletDb::update_chain_tip` will add additional ranges beyond this height, which might include a `Verify` range that ends up merging with the one added during truncation.
str4d
force-pushed
the
983-transparent-balance-fix
branch
from
6a9ad39 to
b76b028
Compare
|
Force-pushed to bring in changes from #988. |
No description provided.