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

Add and use a function for mandatory checkpoint #2314

Merged
merged 5 commits into from
Jun 18, 2021

Conversation

oxarbitrage
Copy link
Contributor

Motivation

We want to refactor the mandatory checkpoint code into a function. If merged this will close #2305

Solution

Add a function into Network implementation. Use it.

Review

Anyone can review, is a minor cleanup change.

Reviewer Checklist

  • Code implements Specs and Designs
  • Tests for Expected Behaviour
  • Tests for Errors

Copy link
Contributor

@teor2345 teor2345 left a comment

Choose a reason for hiding this comment

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

@oxarbitrage
Copy link
Contributor Author

Apologies, the ticket was mentioning only #1846 so i missed those changes. Will fix.

@teor2345
Copy link
Contributor

When you do the fix, can you also fix the clippy warnings as well?

@oxarbitrage
Copy link
Contributor Author

When you do the fix, can you also fix the clippy warnings as well?

Sure, i think i missed that because since some time there are a lot of clippy warnings in the networking code and other unrelated code. I know is unrelated here but do we have a ticket for that ?

@oxarbitrage
Copy link
Contributor Author

I also renamed a lot of acceptance tests (01a3438) that were syncing up to canopy in favour of syncing up to the mandatory checkpoint. I hope I didn't go too far.

@teor2345
Copy link
Contributor

When you do the fix, can you also fix the clippy warnings as well?

Sure, i think i missed that because since some time there are a lot of clippy warnings in the networking code and other unrelated code. I know is unrelated here but do we have a ticket for that ?

There aren't any clippy warnings in Zebra's CI on the current main branch. Our CI runs clippy on every feature in every Zebra crate, including test code. PRs can't merge until they pass clippy.

We run clippy on:

  • Linux
  • rustc 1.52.1

Is your Rust install up to date? Sometimes warnings get fixed or removed from clippy.
Are your PRs based off an old version of the main branch?

@teor2345
Copy link
Contributor

I also renamed a lot of acceptance tests (01a3438) that were syncing up to canopy in favour of syncing up to the mandatory checkpoint. I hope I didn't go too far.

This is great, feel free to update function names or docs, but they're not strictly required for this change.

@oxarbitrage
Copy link
Contributor Author

In regards to clippy, it must be something local (https://gist.github.com/oxarbitrage/8d741c1231335e97c50a06fc9b94ca65). Will research more.

@teor2345
Copy link
Contributor

In regards to clippy, it must be something local (https://gist.github.com/oxarbitrage/8d741c1231335e97c50a06fc9b94ca65). Will research more.

Your PR branch is up to date with our main branch.

What version of rustc are you using?

@oxarbitrage
Copy link
Contributor Author

In regards to clippy, it must be something local (https://gist.github.com/oxarbitrage/8d741c1231335e97c50a06fc9b94ca65). Will research more.

Nevermind, i was in rust 1.51. Thanks.

Copy link
Contributor

@teor2345 teor2345 left a comment

Choose a reason for hiding this comment

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

This looks good, we just need to update some feature names in the GitHub workflows as well.

zebra-chain/src/parameters/network.rs Outdated Show resolved Hide resolved
Comment on lines +63 to +66
test_sync_to_mandatory_checkpoint_mainnet = []
test_sync_to_mandatory_checkpoint_testnet = []
test_sync_past_mandatory_checkpoint_mainnet = []
test_sync_past_mandatory_checkpoint_testnet = []
Copy link
Contributor

Choose a reason for hiding this comment

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

This change is ok, but it also needs an update to the GitHub workflows:
https://github.com/ZcashFoundation/zebra/blob/main/.github/workflows/regenerate-stateful-test-disks.yml#L68
https://github.com/ZcashFoundation/zebra/blob/main/.github/workflows/regenerate-stateful-test-disks.yml#L69
https://github.com/ZcashFoundation/zebra/blob/main/.github/workflows/test.yml#L72

If you're using fastmod, it skips hidden files and directories like .github by default.
You can use fastmod --hidden to change them as well, but you'll need fastmod version 0.4.2.

oxarbitrage and others added 2 commits June 16, 2021 11:49
Co-authored-by: teor <teor@riseup.net>
Copy link
Contributor

@teor2345 teor2345 left a comment

Choose a reason for hiding this comment

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

Looks good, let's merge

@teor2345 teor2345 merged commit 544d182 into ZcashFoundation:main Jun 18, 2021
teor2345 added a commit that referenced this pull request Jun 28, 2021
This is a bugfix on PR #2314, which changed the name of the test
function in the code, but didn't change the CI workflow.
teor2345 added a commit that referenced this pull request Jun 29, 2021
This is a bugfix on PR #2314, which changed the name of the test
function in the code, but didn't change the CI workflow.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Mandatory checkpoint refactor
2 participants