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.

Sign up for GitHub

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

Jump to bottom

change(consensus): Refactor production code for network consensus rules to Network methods #8340

Merged
merged 24 commits into from
Mar 12, 2024
Merged

change(consensus): Refactor production code for network consensus rules to Network methods #8340

merged 24 commits into from
Mar 12, 2024

Conversation

zancas
Copy link
Contributor

@zancas zancas commented Mar 5, 2024 •
edited by arya2
Loading

Supercedes: #8297
Step 2 of: #7968 (comment)

Closes #8326.

Motivation

The goal of this PR is to factor code into zebra-chain::parameters::network::Network.

This will encapsulate network-variant specific behavior behind methods that are implemented on the Network enum.

Developers can then depend on methods like: activation_height returning the appropriate values for the Network they are using.

This convenience can then be extended to more types of networks like custom testnets with mainnet parameters, or parameters that are different from either mainnet or testnet (e.g. the "regtest" case).

PR Author Checklist

Check before marking the PR as ready for review:

  • Will the PR name make sense to users?
  • Does the PR have a priority label?
  • Have you added or updated tests?
  • Is the documentation up to date?
For significant changes:
  • Is there a summary in the CHANGELOG?
    - [ ] Can these changes be split into multiple PRs?

If a checkbox isn't relevant to the PR, mark it as done.

Specifications

Complex Code or Requirements

Solution

Testing

Review

Reviewer Checklist

Check before approving the PR:

  • Does the PR scope match the ticket?
  • Are there enough tests to make sure it works? Do the tests cover the PR motivation?
  • Are all the PR blockers dealt with?
    PR blockers can be dealt with in new tickets or PRs.

And check the PR Author checklist is complete.

Follow Up Work

Minor cleanups

AloeareV and others added 14 commits February 29, 2024 13:53
@AloeareV
begin refactor suggested as "step 2": ZcashFoundation#7968 (comment)
cf70754 
Squashed from multiple commits to enable partial rebase
@AloeareV
break out more little traits
72fe76c
@zancas @AloeareV
add activation implementation leveraging From<Network> for lrz::cons::
2c5dcbf
@zancas @AloeareV
for transfer of ownership I cannot return a type that's owned by the …
ad8530d 
…method
@zancas @AloeareV
hrp_sapling_extended_full_viewing_key
a43134a
@zancas @AloeareV
complete implementation of interface of Parameters on Network reuse P…
950a3b6 
…arameters on zcash Network where possible
@zancas @AloeareV
move doc-comments to trait declarations (from impls)
74770c6
@AloeareV
Simplify/complete Parameters impl for Network
e6d874b
@AloeareV
Add checkpoint_list method, move documentation, etc
aee311d
@AloeareV
Merge branch 'main' into method_per_rule_smaller_traits
78d028d
@AloeareV
move last match network to inside network method
f561645
@AloeareV
add back comment
afc09f3
@AloeareV
use zcash_address for parameter types in zebra-chain
72f26c6
@AloeareV
use inherent methods instead of big parameters passthrough
cc4dda4
@zancas zancas mentioned this pull request Mar 5, 2024
Closed
9 tasks
@zancas zancas marked this pull request as ready for review March 7, 2024 17:09
@zancas zancas requested review from a team as code owners March 7, 2024 17:09
@zancas zancas requested review from upbqdn and removed request for a team March 7, 2024 17:09
@github-actions github-actions bot added the C-trivial Category: A trivial change that is not worth mentioning in the CHANGELOG label Mar 7, 2024
@arya2 arya2 self-requested a review March 11, 2024 21:24
arya2
arya2 previously approved these changes Mar 12, 2024
View reviewed changes
Copy link
Contributor

@arya2 arya2 left a comment

Choose a reason for hiding this comment

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

Thank you!

@arya2 arya2 changed the title Method per rule change(network): Refactor production code for network consensus rules to Network methods Mar 12, 2024
@arya2 arya2 added A-consensus Area: Consensus rule updates C-enhancement Category: This is an improvement A-network Area: Network protocol updates or fixes C-tech-debt Category: Code maintainability issues extra-reviews This PR needs at least 2 reviews to merge P-Medium ⚡ and removed C-trivial Category: A trivial change that is not worth mentioning in the CHANGELOG extra-reviews This PR needs at least 2 reviews to merge labels Mar 12, 2024
@arya2 arya2 dismissed their stale review March 12, 2024 20:44

found some duplicate code

arya2
arya2 approved these changes Mar 12, 2024
View reviewed changes
Copy link
Contributor

@arya2 arya2 left a comment •
edited
Loading

Choose a reason for hiding this comment

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

This still looks good, I left a few comments for minor doc cleanups, none of them are blockers.

I thought the conversion from Network to Magic was duplicated for a moment, but it was just a confusing diff.

zebra-chain/src/parameters/network/tests/prop.rs Show resolved Hide resolved
zebra-chain/Cargo.toml Show resolved Hide resolved
zebra-consensus/src/parameters/subsidy.rs Show resolved Hide resolved
zebra-consensus/src/parameters/subsidy.rs Show resolved Hide resolved
zebra-network/src/protocol/external/types.rs Show resolved Hide resolved
@arya2 arya2 changed the title change(network): Refactor production code for network consensus rules to Network methods change(consensus): Refactor production code for network consensus rules to Network methods Mar 12, 2024
@arya2
Copy link
Contributor

arya2 commented Mar 12, 2024

@Mergifyio refresh

@mergify Mergify
Copy link
Contributor

mergify bot commented Mar 12, 2024

refresh

✅ Pull request refreshed

@mergify mergify bot merged commit 3bef54b into ZcashFoundation:main Mar 12, 2024
156 checks passed
@arya2 arya2 added this to the Regtest Network support milestone Mar 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@arya2 arya2 arya2 approved these changes

@upbqdn upbqdn Awaiting requested review from upbqdn upbqdn is a code owner automatically assigned from ZcashFoundation/cryptographic-reviewers

Assignees
No one assigned
Labels
A-consensus Area: Consensus rule updates A-network Area: Network protocol updates or fixes C-enhancement Category: This is an improvement C-tech-debt Category: Code maintainability issues P-Medium ⚡
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Refactor production code that matches on Testnet to use a new Network method for each consensus rule
3 participants
@zancas @arya2 @AloeareV