Skip to content

CompactBlock parsing and scanning#114

Merged
str4d merged 15 commits into
zcash:masterfrom
str4d:compact-blocks
Oct 9, 2019
Merged

CompactBlock parsing and scanning#114
str4d merged 15 commits into
zcash:masterfrom
str4d:compact-blocks

Conversation

@str4d

@str4d str4d commented Aug 22, 2019

Copy link
Copy Markdown
Contributor

No description provided.

@daira daira left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utACK

@str4d

str4d commented Sep 18, 2019

Copy link
Copy Markdown
Contributor Author

Rebased on master to fix merge conflicts in Cargo.lock after #127.

@str4d

str4d commented Sep 18, 2019

Copy link
Copy Markdown
Contributor Author

Rebased on master to fix a bug in the tests after the API changes in #109.

@str4d

str4d commented Sep 18, 2019

Copy link
Copy Markdown
Contributor Author

Force-pushed to undo the --include-ignored change I made during the last rebase, because it turns out we can't use that flag outside of the nightly compiler (and thus need to run ignored tests separately).

@codecov-io

Copy link
Copy Markdown

Codecov Report

❗ No coverage uploaded for pull request base (master@d2da9cf). Click here to learn what that means.
The diff coverage is 31.81%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master     #114   +/-   ##
=========================================
  Coverage          ?   55.96%           
=========================================
  Files             ?       42           
  Lines             ?     6082           
  Branches          ?        0           
=========================================
  Hits              ?     3404           
  Misses            ?     2678           
  Partials          ?        0
Impacted Files Coverage Δ
zcash_client_backend/src/proto/mod.rs 31.81% <31.81%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d2da9cf...7d1b442. Read the comment docs.

@defuse defuse left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utACK.

API comments:

This interacts with how light wallets handle reorgs, which we want to make as easy as possible so they don't introduce double-spend vulnerabilities with bad reorg handling. With this API, the most obvious way is to keep a copy of the state from N blocks back and rewind to that point then scan_blocks up to the current tip. The caller could forget to revert parts of the state e.g. the nullifier set. The caller could also pass blocks in the wrong order, blocks from a new chain that suddenly grew longer, etc.

We could take responsibility for these problems at this layer by providing a stateful block scanner, one that checks the blocks form a valid chain (prevhash, PoW), automatically keeps track of the witnesses and transactions, and implements rolling-back for reorgs. Light wallets would still have to include some logic for reorgs (at least to refresh the UI to hide payments that disappeared) but if they rely on a stateful scanner as the source of truth when calculating balance, payments received, etc. there's less chance for them to introduce vulnerabilities. There are some trade-offs, like if they want to store their own metadata with each transaction or index them for search, the stateful scanner we provide might not be flexible enough.

(I'm not suggesting that happen in this PR, it would be a higher-level interface that uses scan_block.)

Will the caller ever want to delete viewing keys? Using the key index means that's not possible without screwing up all of the account numbers.

It's outside the scope of this PR, but parse_note_plaintext_without_memo which is called by try_sapling_compact_note_decryption probably leaks some info to local side-channels, maybe we should document that somewhere?

cargo fmt does not build the code, and running it in a fresh clone of
the codebase will fail because the protobuf code has not been generated.
@str4d

str4d commented Oct 9, 2019

Copy link
Copy Markdown
Contributor Author

Rebased on master to fix merge conflicts caused by #138.

@str4d

str4d commented Oct 9, 2019

Copy link
Copy Markdown
Contributor Author

Thanks for the API comments @defuse 🙂 I will merge this PR as-is and then address the comments in subsequent issues.

@str4d str4d merged commit 2cd8a7f into zcash:master Oct 9, 2019
@str4d str4d deleted the compact-blocks branch October 9, 2019 19:11
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

Successfully merging this pull request may close these issues.

4 participants