Skip JoinSplit verification before the last checkpoint #1892

str4d · 2016-11-23T04:45:56Z

Part of #1749

str4d · 2016-11-23T04:48:06Z

Here's the result of a rescan:

- Start of CheckBlock in 0.001714
  - Checked transactions in 0.035483
- Start of CheckBlock in 0.002218
  - Checked transactions in 0.042605
- Start of CheckBlock in 0.002822
  - Checked transactions in 0.000258
UpdateTip: new best=0000001354f18d0da38f6f893c4ac71da5de36d0d9a8741e30df43a4562ee5f9  height=700  log2_work=34.557899  tx=1325  date=2016-10-29 02:49:56 progress=0.002277  cache=0.5MiB(1098tx)
  - Prepared to increment witnesses in 0
  - Incremented witnesses in 1e-06
  - Updated witness heights in 0
  - Wrote witness cache in 0.006148
- Start of CheckBlock in 0.001469
  - Checked transactions in 0.146572
- Start of CheckBlock in 0.001838
  - Checked transactions in 0.144696
- Start of CheckBlock in 0.001653
  - Checked transactions in 0.000691
UpdateTip: new best=0000000d4d229617e388f17ce1420c0ca0e2a75a1023b7d6e733ae7f12512d73  height=701  log2_work=34.565803  tx=1332  date=2016-10-29 02:54:36 progress=0.002289  cache=0.5MiB(1103tx)
  - Prepared to increment witnesses in 0
  - Incremented witnesses in 4e-06
  - Updated witness heights in 0
  - Wrote witness cache in 0.006986
- Start of CheckBlock in 0.00576
  - Checked transactions in 0.049981
- Start of CheckBlock in 0.001385
  - Checked transactions in 0.041102
- Start of CheckBlock in 0.00124
  - Checked transactions in 0.000189
UpdateTip: new best=0000000ad92cc5846397b6816b27b4658d03c4c7b9916fb85b0624f0abdb279f  height=702  log2_work=34.573718  tx=1336  date=2016-10-29 02:57:59 progress=0.002296  cache=0.5MiB(1107tx)
  - Prepared to increment witnesses in 1e-06
  - Incremented witnesses in 3e-06
  - Updated witness heights in 0
  - Wrote witness cache in 0.006816

Two observations:

The speedup is clearly visible
It seems that CheckBlock() gets called three times for every block loaded from disk into the chain during a -reindex. The last of these is in ConnectBlock(); I have yet to identify the other two.

str4d · 2016-11-23T04:57:57Z

Okay, -reindex calls LoadExternalBlockFile(), to indirectly load the blocks in via ProcessNewBlock(). The three calls to CheckBlock() are:

At the start of ProcessNewBlock().
In AcceptBlock() (called by ProcessNewBlock()).
In ConnectBlock() (part of the callstack of ActivateBestChain(), which is called by ProcessNewBlock()).

So we either need to add checkpoint checks in all those locations, or shift JoinSplit verification to e.g. ContextualCheckInputs() (in which case JoinSplits will instead be verified when transactions are added to the mempool, and when ConnectBlock() is called after the last checkpoint).

ebfull · 2016-11-23T14:18:48Z

I want to pass in a verification context object to CheckTransaction which allows us to be explicit about verification behavior. This helps especially when we're dealing with batch verification.

str4d · 2016-12-07T04:33:23Z

Force-pushed to replace the old commit with one based on #1919. This PR is now somewhat more verbose, but also much more explicit about what the verification conditions are, which IMHO is a good thing.

str4d · 2016-12-07T05:22:19Z

Here's the log output after updating my local copy of the blockchain using this PR and then restarting to cause a rescan of the fetched blocks:

2016-12-07 05:18:51 Verifying last 288 blocks at level 3
2016-12-07 05:18:52 No coin database inconsistencies in last 289 blocks (1100 transactions)
2016-12-07 05:18:52  block index            2059ms
2016-12-07 05:18:52 nFileVersion = 1000350
2016-12-07 05:18:52 Keys: 102 plaintext, 0 encrypted, 102 w/ metadata, 102 total
2016-12-07 05:18:52 ZKeys: 1 plaintext, 0 encrypted, 1 w/metadata, 1 total
2016-12-07 05:18:52  wallet                   14ms
2016-12-07 05:18:52 Rescanning last 2880 blocks (from block 20350)...
2016-12-07 05:19:20  rescan                27777ms

Note that this PR now also only verifies JoinSplit proofs once per block (past the last checkpoint), instead of three times.

ebfull · 2016-12-07T17:23:41Z

Reviewed! I'm happy with this. Let's wait until the other PR merges, and then rebase this one.

I think @bitcartel will likely suggest more comments though so that we don't break some assumptions later.

bitcartel · 2016-12-08T04:07:00Z

As part of review we should rebase on #1904 and perform -reindex test due to issue seen here: #1904 (comment)

bitcartel · 2016-12-08T04:16:35Z

The problem is reproducible:

git checkout 1749-disable-js-verification-before-checkpoints
git rebase 1749-write-witness-cache-with-best-block
make
./zcashd -daemon -reindex

zcashd: wallet/wallet.cpp:762: void CWallet::DecrementNoteWitnesses(const CBlockIndex*): Assertion `(nd->witnessHeight == -1) || (nd->witnessHeight == pindex->nHeight)' failed.

ebfull · 2016-12-08T04:38:02Z

@bitcartel is it #1919 causing this bug?

bitcartel · 2016-12-08T05:26:48Z

@ebfull No, it's not #1919 causing the problem. Reindex works fine with #1919 checked out.

str4d · 2016-12-08T11:09:48Z

#1919 doesn't cause the problem on its own, and this PR doesn't change anything to do with the witness cache. So it must be a race condition, only encountered when reindexing is fast enough. But that doesn't make sense either, because the bug occurs during reindexing (confirmation @bitcartel?) and everything should be occurring in series...

bitcartel · 2016-12-08T18:54:20Z

@str4d Yep, during reindexing. I haven't been able to test if the problem occurs with a rescan.

@arielgabizon

Isolate verification to a ProofVerifier context object that allows verification behavior to be tuned by the caller. This is an alternative foundation for #1892, i.e., #1892 will have to be changed if this PR is accepted. I think this is a safer approach because it allows us to isolate verification behavior to a single object. This will come in handy when @arielgabizon finishes the batching code.

Part of zcash#1749

str4d · 2016-12-09T08:08:59Z

Rebased on master to pick up the final changes from #1919.

str4d · 2016-12-09T11:51:10Z

I am confident that this PR is not the cause of the issue (see #1904 (comment)), but I won't merge it yet to let @bitcartel confirm.

@bitcartel please merge this once you are satisfied.

bitcartel · 2016-12-09T18:40:09Z

Locally, I rebased this PR on top of the fixed #1904 which I had rebased on master and the problem is fixed. @str4d Once #1904 is merged, please rebase this PR on master, and I'll test again to make sure everything is fine.

ebfull · 2016-12-09T22:23:46Z

@zkbot tests on merge, so there's no need to rebase anything, just r+ it

zkbot · 2016-12-09T22:23:47Z

📌 Commit 6fb8d0c has been approved by ebfull

zkbot · 2016-12-09T22:23:50Z

⌛ Testing commit 6fb8d0c with merge b8f3461...

…points, r=ebfull Skip JoinSplit verification before the last checkpoint Part of #1749

zkbot · 2016-12-09T22:27:27Z

💔 Test failed - zcash

edit by @ebfull: i killed the builder.

ebfull · 2016-12-09T22:27:42Z

@zkbot r-

bitcartel · 2016-12-09T23:08:10Z

When rebasing on master, I see the assertion again:

wallet/wallet.cpp:779: void CWallet::DecrementNoteWitnesses(const CBlockIndex*): Assertion `nWitnessCacheSize >= nd->witnesses.size()'
because #1904 has been merged and an issue has reappeared since merging.

When not rebasing, and testing this branch as it is, the assertion is not triggered.

str4d · 2016-12-10T01:22:18Z

The bug was definitely in #1904, not this PR. I will fix it in a new PR.

@zkbot r+

zkbot · 2016-12-10T01:22:18Z

📌 Commit 6fb8d0c has been approved by str4d

str4d · 2016-12-10T01:25:06Z

@zkbot r-

str4d · 2016-12-10T01:25:38Z

Will have to merge this after the fix, to get Buildbot to trigger a build.

Psixa18 · 2016-12-12T21:28:20Z

Release 1.0.4 fast or just release as it is right now, please, do not overdue more! I have impression like you don't even realize how important the fixed Z transactions are for the market, miners and Zcash itself, briefly for Zcash value! zk-SNARK is a masterpiece crown, without it nobody can see who is the real king!

bitcartel · 2016-12-13T05:32:03Z

ACK. Reindexing was fine (this PR rebased on #1933)

str4d · 2016-12-13T10:35:17Z

@zkbot r+

zkbot · 2016-12-13T10:35:17Z

📌 Commit 6fb8d0c has been approved by str4d

str4d · 2016-12-13T10:36:21Z

@zkbot retry

…points, r=str4d Skip JoinSplit verification before the last checkpoint Part of #1749

zkbot · 2016-12-13T10:36:25Z

⌛ Testing commit 6fb8d0c with merge 5c47d62...

zkbot · 2016-12-13T11:25:57Z

☀️ Test successful - zcash

str4d added A-consensus Area: Consensus rules I-performance Problems and improvements with respect to performance S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 23, 2016

str4d added this to the 1.0.4 milestone Nov 23, 2016

ebfull mentioned this pull request Dec 5, 2016

Isolate verification to a ProofVerifier context object that allows verification behavior to be tuned by the caller. #1919

Merged

str4d force-pushed the 1749-disable-js-verification-before-checkpoints branch from 9bdeea2 to 7972a86 Compare December 7, 2016 04:31

bitcartel mentioned this pull request Dec 8, 2016

Write witness caches when writing the best block #1904

Merged

Skip JoinSplit verification before the last checkpoint

6fb8d0c

Part of zcash#1749

str4d force-pushed the 1749-disable-js-verification-before-checkpoints branch from 7972a86 to 6fb8d0c Compare December 9, 2016 08:08

zkbot pushed a commit that referenced this pull request Dec 9, 2016

Auto merge of #1892 - str4d:1749-disable-js-verification-before-check…

b8f3461

…points, r=ebfull Skip JoinSplit verification before the last checkpoint Part of #1749

str4d removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Dec 10, 2016

zkbot pushed a commit that referenced this pull request Dec 13, 2016

Auto merge of #1892 - str4d:1749-disable-js-verification-before-check…

5c47d62

…points, r=str4d Skip JoinSplit verification before the last checkpoint Part of #1749

zkbot merged commit 6fb8d0c into zcash:master Dec 13, 2016

str4d deleted the 1749-disable-js-verification-before-checkpoints branch January 27, 2023 16:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Skip JoinSplit verification before the last checkpoint #1892

Skip JoinSplit verification before the last checkpoint #1892

str4d commented Nov 23, 2016

str4d commented Nov 23, 2016

str4d commented Nov 23, 2016

ebfull commented Nov 23, 2016

str4d commented Dec 7, 2016

str4d commented Dec 7, 2016

ebfull commented Dec 7, 2016

bitcartel commented Dec 8, 2016

bitcartel commented Dec 8, 2016

ebfull commented Dec 8, 2016

bitcartel commented Dec 8, 2016

str4d commented Dec 8, 2016

bitcartel commented Dec 8, 2016

str4d commented Dec 9, 2016

str4d commented Dec 9, 2016

bitcartel commented Dec 9, 2016 •

edited

Loading

ebfull commented Dec 9, 2016

zkbot commented Dec 9, 2016

zkbot commented Dec 9, 2016

zkbot commented Dec 9, 2016 •

edited by ebfull

Loading

ebfull commented Dec 9, 2016

bitcartel commented Dec 9, 2016 •

edited

Loading

str4d commented Dec 10, 2016

zkbot commented Dec 10, 2016

str4d commented Dec 10, 2016

str4d commented Dec 10, 2016

Psixa18 commented Dec 12, 2016

bitcartel commented Dec 13, 2016

str4d commented Dec 13, 2016

zkbot commented Dec 13, 2016

str4d commented Dec 13, 2016

zkbot commented Dec 13, 2016

zkbot commented Dec 13, 2016

Skip JoinSplit verification before the last checkpoint #1892

Skip JoinSplit verification before the last checkpoint #1892

Conversation

str4d commented Nov 23, 2016

str4d commented Nov 23, 2016

str4d commented Nov 23, 2016

ebfull commented Nov 23, 2016

str4d commented Dec 7, 2016

str4d commented Dec 7, 2016

ebfull commented Dec 7, 2016

bitcartel commented Dec 8, 2016

bitcartel commented Dec 8, 2016

ebfull commented Dec 8, 2016

bitcartel commented Dec 8, 2016

str4d commented Dec 8, 2016

bitcartel commented Dec 8, 2016

str4d commented Dec 9, 2016

str4d commented Dec 9, 2016

bitcartel commented Dec 9, 2016 • edited Loading

ebfull commented Dec 9, 2016

zkbot commented Dec 9, 2016

zkbot commented Dec 9, 2016

zkbot commented Dec 9, 2016 • edited by ebfull Loading

ebfull commented Dec 9, 2016

bitcartel commented Dec 9, 2016 • edited Loading

str4d commented Dec 10, 2016

zkbot commented Dec 10, 2016

str4d commented Dec 10, 2016

str4d commented Dec 10, 2016

Psixa18 commented Dec 12, 2016

bitcartel commented Dec 13, 2016

str4d commented Dec 13, 2016

zkbot commented Dec 13, 2016

str4d commented Dec 13, 2016

zkbot commented Dec 13, 2016

zkbot commented Dec 13, 2016

bitcartel commented Dec 9, 2016 •

edited

Loading

zkbot commented Dec 9, 2016 •

edited by ebfull

Loading

bitcartel commented Dec 9, 2016 •

edited

Loading