-
Notifications
You must be signed in to change notification settings - Fork 111
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
Legacy chain check and tests #2366
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! I added a few suggestions to the code, and an example of a different way to organize the tests, but they're all optional 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks like a good start - the overall check and structure is pretty good.
Before the next review, let's try to:
- fix bugs in the legacy chain check
- simplify the code and module structure
- change the legacy chain check and proptest strategies so they are easier to test
I also made some suggestions to improve the panic message. We could make them into another ticket. Or if you have time, you could do them after the other changes are done.
I think is ready for a second round of review. The panic changes were not done, i think we should do them in a new ticket with lower priority. I didn't updated #1841 yet, i think the rest is more or less covered even if sometimes not exactly as suggested. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we're pretty good here, just a few things we could tweak, and a small bug fix.
You might want to check with @conradoplg about the chain proptest strategies, he's also fixing some issues.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good now, but it changes the cached state tests, so it is blocked by #2402.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The blockers have all merged.
There's no easy way to test this branch merged with the latest main
, so I'm just going to go ahead and merge, and then check that it works.
If it fails someone should revert and re-open the PR.
This appears to work locally on mainnet and testnet, with and without cached state, and before and after the mandatory checkpoint. |
Motivation
We need to have a check after Nu5 activation when zebra starts to make sure we are not following a legacy chain. Close #2308
Solution
Implement what is detailed in the ticket: #2308
Review
I know a lot here can be improved so i will like @jvff to make an initial review before passing it into @teor2345 later on.
Reviewer Checklist