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 an empty NetworkParameters struct to Network::Testnet #7968

Closed
Tracked by #7845
arya2 opened this issue Nov 21, 2023 · 3 comments · Fixed by #8368
Closed
Tracked by #7845

Add an empty NetworkParameters struct to Network::Testnet #7968

arya2 opened this issue Nov 21, 2023 · 3 comments · Fixed by #8368
Assignees
Labels
A-consensus Area: Consensus rule updates A-network Area: Network protocol updates or fixes C-feature Category: New features C-testing Category: These are tests good first issue

Comments

@arya2
Copy link
Contributor

arya2 commented Nov 21, 2023

Motivation

Zebra currently hard-codes some consensus parameters that may need to be changed for testing.

Depends-On: #8325, #8326

Possible Design

  • Add an empty and optional TestnetParameters struct in Network::Testnet
  • Add a Network::new_testnet() method that returns Network::Testnet { ..default_testnet_params }
    • Mass replace any remaining instances of Testnet in tests with Network::new_testnet()

Example (with fields that shouldn't be added yet):

Testnet(Arc<NetworkParameters { 
   cache_name,
   genesis_hash,
   network_id,
   default_port,
   activation_heights
>)
@mpguerra mpguerra added this to Zebra Nov 21, 2023
@github-project-automation github-project-automation bot moved this to 🆕 New in Zebra Nov 21, 2023
@arya2 arya2 added A-consensus Area: Consensus rule updates P-Medium ⚡ I-usability Zebra is hard to understand or use A-network Area: Network protocol updates or fixes NU-6 Network Upgrade: NU6 specific tasks C-testing Category: These are tests C-feature Category: New features labels Nov 21, 2023
@arya2 arya2 self-assigned this Nov 21, 2023
@mergify mergify bot closed this as completed in #7955 Nov 21, 2023
@github-project-automation github-project-automation bot moved this from 🆕 New to ✅ Done in Zebra Nov 21, 2023
@arya2 arya2 reopened this Nov 21, 2023
@mpguerra mpguerra moved this from Done to Product Backlog in Zebra Jan 16, 2024
@mpguerra mpguerra added this to the NU6 milestone Jan 19, 2024
@AloeareV
Copy link
Contributor

1. refactor tests to use `is_mainnet()` to look up test vectors using new functions in `zebra-test`, rather than matching on `Testnet`. This should be quick and easy to approve since it's only test code, and there are only mainnet and default public testnet test vectors. (No custom testnet vectors.)

If I'm going to implement this, is the intent here to replace the match network blocks with if network.is_mainnet() blocks? New functions in zebra-test like some kind of fn test_vector_for_network that gets offloaded to?

AloeareV added a commit to zingolabs/zebra that referenced this issue Feb 22, 2024
Squashed from multiple commits to enable partial rebase
@arya2
Copy link
Contributor Author

arya2 commented Feb 22, 2024

If I'm going to implement this, is the intent here to replace the match network blocks with if network.is_mainnet() blocks?

We want to do this to minimize the changes in the following PRs where the Testnet variant is modified, but this issue is for adding an empty struct in the Testnet variant of Network. Replacing match network blocks with if network.is_mainnet() in tests should precede this issue as an early step in #7845, but we're still planning #7845 so there isn't an open issue for this yet.

We decided PR #7924 was too large to test or review accurately, and because this is a high-risk area of the code, we want to divide the changes across multiple PRs, where each PR adds one configurable parameter and Network method with unit tests for its changes.

We also discussed #1135 around the time that #7924 was opened, and decided to add any configurable parameters to the Testnet variant instead of adding a new variant or refactoring Network into a trait to reduce scope.

@arya2 arya2 changed the title Add an empty NetworkParameters struct to Network::Testnet Add an empty TestnetParameters struct to Network::Testnet Feb 26, 2024
@arya2 arya2 added the S-blocked Status: Blocked on other tasks label Feb 26, 2024
@arya2 arya2 removed their assignment Feb 26, 2024
@arya2
Copy link
Contributor Author

arya2 commented Feb 26, 2024

I split the "Multiple PRs" section of this issue into a separate issues in #7845.

@arya2 arya2 changed the title Add an empty TestnetParameters struct to Network::Testnet Add an empty NetworkParameters struct to Network::Testnet Feb 26, 2024
AloeareV added a commit to AloeareV/zebra that referenced this issue Feb 29, 2024
Squashed from multiple commits to enable partial rebase
@mpguerra mpguerra modified the milestones: NU6, Regtest Mar 6, 2024
mergify bot pushed a commit that referenced this issue Mar 12, 2024
…es to `Network` methods (#8340)

* begin refactor suggested as "step 2": #7968 (comment)
Squashed from multiple commits to enable partial rebase

* break out more little traits

* add activation implementation leveraging From<Network> for lrz::cons::

* for transfer of ownership I cannot return a type that's owned by the method

* hrp_sapling_extended_full_viewing_key

* complete implementation of interface of Parameters on Network reuse Parameters on zcash Network where possible

* move doc-comments to trait declarations (from impls)

* Simplify/complete Parameters impl for Network

* Add checkpoint_list method, move documentation, etc

* move last match network to inside network method

* add back comment

* use zcash_address for parameter types in zebra-chain

* use inherent methods instead of big parameters passthrough

* revert to implementation of From on zcash_primitives::..::Network vs &zcash_prim...

* move match

* add test to block maximum time rule

* update changelog

* finish porting target_difficutly_limit

* remove obscelete code comment

* revert non-functional change

* finish migrating target_difficulty_limit, checkpoint_list

* update changelog

---------

Co-authored-by: Hazel OHearn <gygaxis@zingolabs.org>
@arya2 arya2 self-assigned this Mar 20, 2024
@arya2 arya2 removed S-blocked Status: Blocked on other tasks I-usability Zebra is hard to understand or use labels Mar 20, 2024
@mpguerra mpguerra moved this from Product Backlog to Sprint Backlog in Zebra Mar 20, 2024
@mpguerra mpguerra moved this from Sprint Backlog to Review/QA in Zebra Apr 10, 2024
@mergify mergify bot closed this as completed in #8368 Apr 17, 2024
@github-project-automation github-project-automation bot moved this from Review/QA to Done in Zebra Apr 17, 2024
@mpguerra mpguerra removed the NU-6 Network Upgrade: NU6 specific tasks label Jul 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-consensus Area: Consensus rule updates A-network Area: Network protocol updates or fixes C-feature Category: New features C-testing Category: These are tests good first issue
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

3 participants