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

Always write the empty root down as the best root, since we may roll back #3463

Merged
merged 1 commit into from Aug 15, 2018

Conversation

5 participants
@ebfull
Copy link
Contributor

ebfull commented Aug 14, 2018

In 3577de83 we started not writing the Sapling empty root down as the "best" anchor because we had changed the encodings and didn't want users who compiled from master to have inconsistent coindb's in the future if the encoding changed again for some reason.

However, if we don't write the empty root down then during rollbacks to Sapling activation we leave the best anchor on disk different from what's in the cache, which will trigger an assertion.

This reverts the change from 3577de83 since we've settled on the encodings.

@daira

This comment has been minimized.

Copy link
Contributor

daira commented Aug 14, 2018

3577de8 also changed the DB_SAPLING_ANCHOR and DB_BEST_SAPLING_ANCHOR codes. Is it okay to keep the 'Z' and 'z' codes now?

@ebfull

This comment has been minimized.

Copy link
Contributor

ebfull commented Aug 14, 2018

Yes, those should stay the same. (Let me be clear; the change to those codes should remain the way they are, we only need to revert this specific logic.)

@daira

daira approved these changes Aug 14, 2018

Copy link
Contributor

daira left a comment

utACK

@daira daira added this to the v2.0.0 milestone Aug 14, 2018

@daira daira added the Sapling label Aug 14, 2018

@bitcartel

This comment has been minimized.

Copy link
Contributor

bitcartel commented Aug 14, 2018

Why is Sprout code touched here?

@ebfull

This comment has been minimized.

Copy link
Contributor

ebfull commented Aug 14, 2018

There's no need for the behavior to be different between the Sprout and Sapling implementations. In this case it won't have any effect since we can't rollback to the empty Sprout anchor.

@str4d

str4d approved these changes Aug 14, 2018

Copy link
Contributor

str4d left a comment

utACK, change justification makes sense.

@ebfull

This comment has been minimized.

Copy link
Contributor

ebfull commented Aug 15, 2018

@zkbot r+

@zkbot

This comment has been minimized.

Copy link
Collaborator

zkbot commented Aug 15, 2018

📌 Commit f791ce0 has been approved by ebfull

@zkbot

This comment has been minimized.

Copy link
Collaborator

zkbot commented Aug 15, 2018

⌛️ Testing commit f791ce0 with merge e868f82...

zkbot added a commit that referenced this pull request Aug 15, 2018

Auto merge of #3463 - ebfull:revert-empty-root, r=ebfull
Always write the empty root down as the best root, since we may roll back

In [`3577de83`](3577de8) we started not writing the Sapling empty root down as the "best" anchor because we had changed the encodings and didn't want users who compiled from master to have inconsistent coindb's in the future if the encoding changed again for some reason.

However, if we don't write the empty root down then during rollbacks to Sapling activation we leave the best anchor on disk different from what's in the cache, which will trigger an assertion.

This reverts the change from `3577de83` since we've settled on the encodings.
@zkbot

This comment has been minimized.

Copy link
Collaborator

zkbot commented Aug 15, 2018

☀️ Test successful - pr-merge
Approved by: ebfull
Pushing e868f82 to master...

@zkbot zkbot merged commit f791ce0 into zcash:master Aug 15, 2018

1 check passed

homu Test successful
Details

@zkbot zkbot added this to Released (Merged in Master) in Zcashd Team via automation Aug 15, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment