Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix spent nullifier detection for non-linear scanning #876

Closed
str4d opened this issue Jul 19, 2023 · 2 comments
Closed

Fix spent nullifier detection for non-linear scanning #876

str4d opened this issue Jul 19, 2023 · 2 comments

Comments

@str4d
Copy link
Contributor

str4d commented Jul 19, 2023

#872 introduced support for non-linear scanning. Clients can call suggest_scan_ranges and fetch blocks in that order, to make unspent notes spendable faster.

However, just after the PR merged I noticed a flaw: if you scan block range Y..Z before block range X..Y, then any notes that were received in the range X..Y and spent in the range Y..Z won't be detected as spent. This is because their nullifiers weren't known at the time Y..Z was scanned (so they weren't being looked for), and the range Y..Z isn't re-checked after X..Y is scanned.

(I noticed this because I tried fetching all given block ranges in reverse order, which led to my zec-sqlite-cli wallet greatly over-reporting balance, which is Bad.)

DAGSync doesn't have this problem by design (see #776 for related comments). But we need a way to ensure we don't have this problem for the pre-DAGSync approach we are retrofitting into the data API.

@str4d
Copy link
Contributor Author

str4d commented Jul 19, 2023

@ebfull and I talked this over in a pairing, and discussed various options. The least-bad option I want to go with is as follows.

  • We add a nullifier_map table to the wallet DB, with three columns (nf, block, tx_index).
  • When scan_cached_blocks is called:
    • For all newly-discovered notes that are considered unspent after scanning (i.e. their spend wasn't also in the scanned range), check the nullifier_map table for a match. If a match is found:
      • Mark the note as spent.
      • Delete that row from nullifier_map if the block column value is at at most 100 blocks before the last-known chain tip height (i.e. is in the "stable" chain region).
    • Check whether the scanned range caused the "fully-scanned" height of the wallet to increase (meaning that this range was effectively linear scanning, and maybe filled in a section causing later block ranges to also be contiguous).
      • If it did, then remove all rows from nullifier_map with block < fully_scanned_height.
      • If it didn't, then add all nullifiers from the scan range to nullifier_map (excluding any nullifiers that had a match within the scan range and would have a block column value at most 100 blocks before the last-known chain tip height).

What this should mean is that if a wallet is mostly linear scanning, but with occasional non-linear jumps forward (e.g. the "update latest shard tree" scan ranges that #872 suggests), we will only store the nullifier map for those non-linear sections.

If a wallet chooses to scan blocks in complete reverse order, then we will end up storing the entire nullifier map from current chain tip to "scan start" (wallet birthday, or the last time we were fully scanned). Worst case this would be storing the entire nullifier set, which could be GiB of data. However, the caller could detect the database size increase and switch to linear scanning until it fills in the remaining range, at which point the nullifier map would be entirely pruned.

@str4d
Copy link
Contributor Author

str4d commented Jul 21, 2023

Fixed in #878.

@str4d str4d closed this as completed in 0f2689b Jul 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant