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

When rewinding, remove insufficiently-validated blocks #3132

Merged
merged 2 commits into from Apr 3, 2018

Conversation

str4d
Copy link
Contributor

@str4d str4d commented Mar 30, 2018

If a block is insufficiently-validated against a particular branch ID, then we
cannot guarantee that even the block header will be valid under the actual
consensus rules the node will want to apply. Instead require that the blocks are
completely re-validated, by removing them from the block index (which is
equivalent to reducing their validity to BLOCK_VALID_UNKNOWN).

Closes #3100.

@str4d str4d added this to the v1.1.0 milestone Mar 30, 2018
@str4d str4d requested review from bitcartel and daira March 30, 2018 23:34
@str4d
Copy link
Contributor Author

str4d commented Mar 30, 2018

Related to #3100, which is tangentially similar.

Copy link
Contributor

@daira daira left a comment

Choose a reason for hiding this comment

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

utACK. Do we have a regression test for the bug?

@str4d
Copy link
Contributor Author

str4d commented Apr 1, 2018

That's what I was blocking on - trying to figure out a nice way to test it. I think I'll have to do the not-so-nice thing where we emulate the "switching clients" behaviour by stopping and starting nodes in an RPC test.

@str4d
Copy link
Contributor Author

str4d commented Apr 2, 2018

I have (what I believe is) an RPC test for RewindBlockIndex, and it shows a bug in this PR. Do not merge yet.

@str4d str4d changed the title When rewinding, reduce block validity to BLOCK_VALID_UNKNOWN When rewinding, remove insufficiently-validated blocks Apr 3, 2018
@str4d str4d requested a review from daira April 3, 2018 00:24
@str4d
Copy link
Contributor Author

str4d commented Apr 3, 2018

This PR now has an RPC test that fails before the fix, and passes afterwards.

@str4d
Copy link
Contributor Author

str4d commented Apr 3, 2018

Force-pushed to fix PyFlakes warning 😄

Copy link
Contributor

@bitcartel bitcartel left a comment

Choose a reason for hiding this comment

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

ACK. I also tested against Federer test datadir, where a Sprout 1.0.15 node rewinds and gets stuck at block 199999, but this PR enables the node to sync successfully with testnet.

Copy link
Contributor

@daira daira left a comment

Choose a reason for hiding this comment

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

ut(ACK+cov) apart from the incorrect debugging messages.

assert_equal(self.nodes[2].getbestblockhash(), block10o)

# Restart node 0 using Sprout instead of Overwinter
print("Switching node 0 from Sprout to Overwinter")
Copy link
Contributor

Choose a reason for hiding this comment

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

from Overwinter to Sprout

assert_equal(self.nodes[0].getbestblockhash(), block10s)

# Restart node 0 using Overwinter instead of Sprout
print("Switching node 0 from Overwinter to Sprout")
Copy link
Contributor

Choose a reason for hiding this comment

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

from Sprout to Overwinter

If a block is insufficiently-validated against a particular branch ID, then we
cannot guarantee that even the block header will be valid under the actual
consensus rules the node will want to apply. Instead require that the blocks are
completely re-validated, by removing them from the block index (which is
equivalent to reducing their validity to BLOCK_VALID_UNKNOWN).
Copy link
Contributor

@daira daira left a comment

Choose a reason for hiding this comment

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

re-ut(ACK+cov)

@daira
Copy link
Contributor

daira commented Apr 3, 2018

@zkbot r+

@zkbot
Copy link
Contributor

zkbot commented Apr 3, 2018

📌 Commit f5007d8 has been approved by daira

@zkbot
Copy link
Contributor

zkbot commented Apr 3, 2018

⌛ Testing commit f5007d8 with merge 3a47b9b...

zkbot added a commit that referenced this pull request Apr 3, 2018
When rewinding, remove insufficiently-validated blocks

If a block is insufficiently-validated against a particular branch ID, then we
cannot guarantee that even the block header will be valid under the actual
consensus rules the node will want to apply. Instead require that the blocks are
completely re-validated, by removing them from the block index (which is
equivalent to reducing their validity to BLOCK_VALID_UNKNOWN).

Closes #3100.
@zkbot
Copy link
Contributor

zkbot commented Apr 3, 2018

☀️ Test successful - pr-merge
Approved by: daira
Pushing 3a47b9b to master...

@zkbot zkbot merged commit f5007d8 into zcash:master Apr 3, 2018
@str4d str4d deleted the 2898-rewind-block-index branch April 3, 2018 20:36
@str4d str4d mentioned this pull request Apr 12, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants