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

Rebuild witness and nullifier caches in wallet if a large reorganisation occurs #1302

Closed
str4d opened this issue Aug 25, 2016 · 10 comments
Closed
Labels
A-wallet Area: Wallet C-bug Category: This is a bug C-cleanup Category: PRs that clean code up or issues documenting cleanup. S-wontfix Status: An issue we have decided not to address.

Comments

@str4d
Copy link
Contributor

str4d commented Aug 25, 2016

In #1233 a cache of incremental witnesses per spendable note was added to the wallet, and a corresponding cache of anchors nullifiers. The size of the cache is intended to be large enough that if a chain reorganisation occurs, the cache will not be exhausted. However, in rare cases (e.g. extended network splits), a large reorganisation might occur, and the wallet will cease to behave correctly if the cache is exhausted. In this case, we should rebuild the cache from scratch.

@str4d str4d added C-bug Category: This is a bug A-wallet Area: Wallet C-cleanup Category: PRs that clean code up or issues documenting cleanup. labels Aug 25, 2016
@str4d str4d added this to the Beta 1 - complete RPC, audit mitigations, other linux support, user bugfixes milestone Aug 25, 2016
@ebfull
Copy link
Contributor

ebfull commented Aug 25, 2016

This can very well be something we don't launch with if the anchor cache in the wallet is large enough.

@str4d
Copy link
Contributor Author

str4d commented Sep 2, 2016

The anchor cache is currently the same size as the coinbase maturity duration, so if we do encounter this issue, we'd also be dealing with the issue of transactions becoming invalid due to non-existent coinbase.

@nathan-at-least
Copy link
Contributor

Let's defer this until #713 is complete, since that will give us a known boundary condition, and may make this unnecessary.

@str4d
Copy link
Contributor Author

str4d commented Jun 20, 2017

I don't believe #713 will make this unnecessary. Forward movement on re-organization happens in chunks of at most 32 blocks (to prevent ActivateBestChain from holding the main lock for too long), so it is possible that two re-orgs could occur in succession that individually are below the re-org limit, but together exceed the cache size. This would require three separate chain forks to occur, with the most recent fork parent under 100 blocks ago, and the second fork parent somewhere between 100 and 200 blocks ago. The node would then need to learn of the more recent chain fork first, followed immediately by the older chain fork.

@str4d str4d added this to the 1.0.15 milestone Jan 25, 2018
@str4d str4d self-assigned this Jan 25, 2018
@str4d str4d added this to Work Queue in Network Upgrade 0 via automation Jan 25, 2018
@str4d str4d added this to 1.0.15: Consensus / Node Rules in Release planning Jan 25, 2018
@str4d str4d removed this from 1.0.15: Consensus / Node Rules in Release planning Feb 22, 2018
@str4d str4d removed this from the v1.0.15 milestone Feb 22, 2018
@str4d str4d added this to 1.1.1: Wallet & DB in Release planning Apr 5, 2018
@daira daira removed this from Work Queue in Network Upgrade 0 Apr 14, 2018
@str4d str4d added this to Sprint Backlog in Arborist Team Apr 26, 2018
@mdr0id mdr0id moved this from Sprint Backlog to Current Sprint in Arborist Team Jun 7, 2018
@str4d str4d removed their assignment Jul 11, 2018
@str4d str4d added this to the v2.0.0 milestone Jul 11, 2018
@str4d str4d changed the title Rebuild witness and anchor caches in wallet if a large reorganisation occurs Rebuild witness and nullifier caches in wallet if a large reorganisation occurs Jul 31, 2018
@str4d str4d moved this from Current Sprint to In Progress in Arborist Team Aug 2, 2018
zkbot added a commit that referenced this issue Aug 8, 2018
Support testnet rollback.

Part of #1302. Closes #2905.
@daira
Copy link
Contributor

daira commented Aug 16, 2018

#3443 demonstrated that it is possible to fix this problem, but it still isn't entirely fixed. Bumping milestone to 2.0.1.

@daira daira modified the milestones: v2.0.0, v2.0.1 Aug 16, 2018
@bitcartel bitcartel moved this from In Progress to Sprint Backlog in Arborist Team Aug 22, 2018
@bitcartel bitcartel moved this from Sprint Backlog to Product Backlog in Arborist Team Aug 22, 2018
@str4d str4d modified the milestones: v2.0.1, v2.0.2 Oct 8, 2018
@arielgabizon
Copy link
Contributor

Ran into this while trying to run the pruning test which does a 288 block reorg #2815 (comment)

@daira daira modified the milestones: v2.0.2, v2.0.3 Nov 30, 2018
@str4d str4d removed this from the v2.0.3 milestone Feb 15, 2019
@jan-matejka
Copy link

Keep running into this zcashd: wallet/wallet.cpp:1288: void CWallet::DecrementNoteWitnesses(const CBlockIndex*): Assertion `nWitnessCacheSize > 0' failed. when testing on regtest node.

@jan-matejka
Copy link

If I understand the issue correctly in my case it is caused by a fixture in my testsuite that "cleans" the chain between executing different testcases by invalidating block at height 1 which often causes invalidating a chain of more than 101 blocks. So increasing WITNESS_CACHE_SIZE by 1000 should solve the issue for me. Should I expect other issues with this patch?

@jan-matejka
Copy link

so I patched the MAX_REORG_LENGTH to increase it by 1000 but I still get the same error when trying to invalidateblock with more than 110 confirmations. Why is beyond me. Might be related to that I was unable to find where is the cache initialized by grepping for the WITNESS_CACHE_SIZE.

@nuttycom nuttycom added the S-wontfix Status: An issue we have decided not to address. label Jan 27, 2023
@nuttycom
Copy link
Contributor

zcashd will no longer perform a reorg of more than 100 blocks

Arborist Team automation moved this from Needs Prioritization to Complete Jan 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-wallet Area: Wallet C-bug Category: This is a bug C-cleanup Category: PRs that clean code up or issues documenting cleanup. S-wontfix Status: An issue we have decided not to address.
Projects
Arborist Team
  
Complete
Release planning
1.1.1: Wallet & DB
Security and Stability
  
Work Queue
Development

No branches or pull requests

7 participants