zcash_client_sqlite: Ensure that Orchard and Sapling checkpoints are always available for the same block heights. #1262
Conversation
nuttycom
force-pushed
the
sqlite_wallet/fix_multi_pool_checkpointing
branch
3 times, most recently
from
47a7f1a to
ae3f56d
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1262 +/- ##
==========================================
+ Coverage 63.26% 63.57% +0.31%
==========================================
Files 121 121
Lines 13509 13646 +137
==========================================
+ Hits 8547 8676 +129
- Misses 4962 4970 +8 ☔ View full report in Codecov by Sentry. |
nuttycom
force-pushed
the
sqlite_wallet/fix_multi_pool_checkpointing
branch
from
cbb01e5 to
47866a7
Compare
|
force-pushed to address comments from code review & disable tests that require test framework changes. |
nuttycom
changed the title
…anned range. In order to support constructing the anchor for multiple pools with a common anchor height, we must be able to checkpoint each note commitment tree (and consequently compute the root) at that height. Since we may not have the information in the tree needed to do so, we require that it be provided. As a bonus, this change makes it possible to improve the UX around spendability, because we will no longer require subtree ranges below received notes to be fully scanned; the inserted frontier provides sufficient information to make them spendable.
nuttycom
force-pushed
the
sqlite_wallet/fix_multi_pool_checkpointing
branch
from
47866a7 to
fb46ff9
Compare
|
force-pushed to rebase on |
…ork behind the `orchard` flag.
nuttycom
force-pushed
the
sqlite_wallet/fix_multi_pool_checkpointing
branch
from
fb46ff9 to
a046088
Compare
| 'a, | ||
| H, | ||
| I: Iterator<Item = &'a BlockHeight>, |
There was a problem hiding this comment.
Minor nit: I think the 'a, I: .. can be inlined by making the argument
checkpoint_heights: impl Iterator<Item = &'_ BlockHeight>,There was a problem hiding this comment.
I fooled around with a couple of things and couldn't get it to work, so went with the simple thing. :)
|
Missing a CHANGELOG addition for this? Ideally it would include not just the changed APIs, but a brief explanation of how to come up with the new |
@AArnott thanks for noticing, I'll fix this up. The conversion you want is https://github.com/zcash/librustzcash/blob/main/zcash_client_backend/src/proto.rs#L297-L307 |
This fixes an error that was causing it to be impossible to make cross-pool transactions when an anchor was needed for each pool, but alternating blocks held only Sapling or only Orchard notes. Prior to this fix, checkpoints were only created in the note commitment tree for a block for whatever pool had activity within that block; for example, if a block contained Orchard actions but no Sapling inputs or outputs, only an Orchard checkpoint was being created.
The fix for this issue requires that the initial tree state for each pool be available for each range of scanned blocks, so that we can be sure that we know the correct note commitment tree to position to checkpoint in that pool for a block that contains no inputs or outputs for the pool. As an ancillary benefit, this then will make the implementation of #982 trivial.