Changed the note scanning algorithm to not require additional context.#2458
Merged
tzemanovic merged 9 commits intomainfrom Feb 22, 2024
Merged
Changed the note scanning algorithm to not require additional context.#2458tzemanovic merged 9 commits intomainfrom
tzemanovic merged 9 commits intomainfrom
Conversation
murisi
force-pushed
the
murisi+jacob/masp-scanning
branch
2 times, most recently
from
623aa1d to
5896be5
Compare
grarco
previously approved these changes
Jan 29, 2024
batconjurer
commented
Jan 29, 2024
Collaborator
Author
batconjurer
left a comment
batconjurer
left a comment
There was a problem hiding this comment.
The fetch commands in the client are still present, so this can't be merged as is. Also, this is missing the batching and caching. I've got that implemented now in another branch, but still needs a bit of debugging.
2 tasks
batconjurer
force-pushed
the
murisi+jacob/masp-scanning
branch
from
0ae203f to
b177507
Compare
grarco
reviewed
Jan 31, 2024
| let last_block_height = query_block(client) | ||
| .await? | ||
| .map_or_else(BlockHeight::first, |block| block.height); | ||
| let last_query_height = last_query_height.unwrap_or(last_block_height); |
Collaborator
There was a problem hiding this comment.
Should we query for the block height only if the arg is None? Just to avoid an extra RPC call since we only use last_block_height here. Anyway this is very minor, we could change it in another PR in case
grarco
previously approved these changes
Jan 31, 2024
Closed
cwgoes
previously approved these changes
Feb 2, 2024
Collaborator
cwgoes
left a comment
cwgoes
left a comment
There was a problem hiding this comment.
Makes sense to me. In the future, we might want to consider additional algorithm improvements such as (possibly dependent on user configuration/instructions):
- scanning from the most recent height backwards instead of from the oldest height forwards
- trial decrypting with a specific viewing key as opposed to all of them, for example if a user knows where they should have received a payment (this applies mostly if we get to a point where this is compute-bound)
Closed
2 tasks
Closed
Closed
tzemanovic
added a commit
that referenced
this pull request
Feb 15, 2024
* origin/murisi+jacob/masp-scanning: [chore]: Fix integration masp test fixtures I don't know [chore] Rebase on main [fix] Some e2e test fixes Moved fetch calls completely from other calls. Updated cli. Fixed tests Added changelog entry. Changed the note scanning algorithm to not require additional context.
Closed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Describe your changes
Simplified the note scanning algorithm so that it does not require a merge operation when new keys are introduced into the wallet. Instead of having two paths in the fetching code: one for when the viewing keys are unchanged but there are new shielded transactions from the network, and the other for when new keys are introduced and transactions must be rescanned from the beginning. Instead of doing that, we now have a single path that starts scanning transactions from the most out of date viewing key going forward to the present. This change increases the set of valid states that a
ShieldedContextcan have and allows its (recoverable) state to be saved more frequently.Indicate on which release or other PRs this topic is based on
Namada v0.30.2
Checklist before merging to
draft