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

add(consensus): Add activation_heights field to NetworkParameters and implement Parameters for Network #8383

Merged
merged 37 commits into from
Apr 19, 2024
Merged

add(consensus): Add activation_heights field to NetworkParameters and implement Parameters for Network #8383

merged 37 commits into from
Apr 19, 2024

Conversation

arya2
Copy link
Contributor

@arya2 arya2 commented Apr 3, 2024 •
edited
Loading

Motivation

We want to have an activation_heights field on NetworkParameters so that we can use different values on Regtest and so that it can be configured for custom Testnets.

Closes #7970.
Closes #8365.
Closes #7840.

Depends-On: #8368

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.

Solution

  • Adds an activation_heights field to NetworkParameters
  • Adds an intermediate struct for deserializing testnet_parameters from Zebra's config
  • Implements Parameters for Network to avoid tricky conversions to zcash_primitives::consensus::Network
    • Uses placeholder values for unknown testnets, we should discuss what to do there before merging this PR

Testing

  • Adds a stored_configs_parsed_correctly test to check if new configs are valid more quickly
  • Updates the stored config for v1.7.0
  • Checks that the zcash_primitives Parameters impl for zebra-chain's Network's method outputs match those of the Parameters impl for zcash_primitives::consensus::Network.

Review

Anyone can 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.

arya2 added 10 commits April 1, 2024 21:45
@arya2
minor cleanup and rename
9db4762
@arya2
Adds an empty NetworkParameters struct to Network::Testnet variant, u…
4b86bd6 
…pdates production code.
@arya2
Updates tests
7533e2f
@arya2
Adds NetworkKind and uses it instead of Network in `HistoryTreePa…
06cf947 
…rts` and `transparent::Address`
@arya2
Adds a [network.testnet_parameters] section to the config, uses `Netw…
4900a20 
…orkKind` as zebra_network::Config::network field type, and converts 'Network' to `NetworkKind` before serializing
@arya2
Applies some suggestions from code review
cb8726b
@arya2
Applies suggestions from code review
d5a807a
@arya2
returns b58 prefix constants directly to remove From<NetworkKind> imp…
d108ec6 
…l for zcash_primitives::consensus::Network
@arya2
Applies more suggestions from code review.
574b1f9
@arya2
moves conversions to zcash_primitives::consensus::Network to where th…
05e41a2 
…ey're used.
@arya2 arya2 added A-consensus Area: Consensus rule updates C-enhancement Category: This is an improvement 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 Apr 3, 2024
@arya2 arya2 self-assigned this Apr 3, 2024
This was linked to issues Apr 3, 2024
Add a field for the network upgrade activation heights to TestnetParameters #7970
Closed
Implement zcash_primitives::consensus::Parameters for Network to pass network to try_sapling_output_recovery() directly #8365
Closed
arya2 and others added 10 commits April 11, 2024 12:36
@arya2 @upbqdn
Apply suggestions from code review
fd3d505 
Co-authored-by: Marek <[email protected]>
@arya2
rename network variables and method names typed as NetworkKind to `…
f04b94b 
…network_kind`
@arya2
use only test block heights for the network associated with them
ea2d001
@arya2
Applies more suggestions from code review.
86cd919
@arya2
Rename NetworkParameters to Parameters and move it a new `testnet…
365b58e 
…` module
@arya2
Merge branch 'main' into network-params
eca7f6b
@arya2
adds activation heights field
60248e3
@arya2
updates stored test config, adds a quicker test for checking that sto…
b9bdf05 
…red configs can be parsed, adds an intermediate representation of activation heights
@arya2
implement Parameters for Network
ad4e7dc
@arya2
Passes &Network directly instead of converting to zp_consensus::Netwo…
4347a80 
…rk where there were conversions
@arya2 arya2 changed the title add(chain): Add activation_heights field to NetworkParameters and implement Parameters for Network add(consensus): Add activation_heights field to NetworkParameters and implement Parameters for Network Apr 12, 2024
@arya2 arya2 added the do-not-merge Tells Mergify not to merge this PR label Apr 15, 2024
@arya2 arya2 mentioned this pull request Apr 15, 2024
Merged
7 tasks
@arya2 arya2 removed the do-not-merge Tells Mergify not to merge this PR label Apr 17, 2024
Base automatically changed from network-params to main April 17, 2024 02:20
upbqdn
upbqdn previously approved these changes Apr 17, 2024
View reviewed changes
zebrad/tests/acceptance.rs Outdated Show resolved Hide resolved
zebra-chain/src/parameters/network.rs Show resolved Hide resolved
zebra-rpc/src/methods/get_block_template_rpcs.rs Outdated Show resolved Hide resolved
zebra-rpc/src/methods/get_block_template_rpcs.rs Outdated Show resolved Hide resolved
zebra-rpc/src/methods/get_block_template_rpcs.rs Outdated Show resolved Hide resolved
zebra-chain/src/parameters/network_upgrade.rs Show resolved Hide resolved
@arya2 arya2 requested a review from upbqdn April 17, 2024 22:59
@arya2 arya2 mentioned this pull request Apr 18, 2024
Merged
9 tasks
upbqdn
upbqdn previously approved these changes Apr 18, 2024
View reviewed changes
zebra-chain/src/parameters/network_upgrade.rs Outdated Show resolved Hide resolved
@arya2 arya2 requested a review from upbqdn April 18, 2024 14:14
mergify bot added a commit that referenced this pull request Apr 18, 2024
@mergify
Merge of #8383
4ed44f4
@mergify mergify bot mentioned this pull request Apr 18, 2024
Closed
@arya2
Copy link
Contributor Author

arya2 commented Apr 18, 2024

@Mergifyio refresh

@mergify Mergify
Copy link
Contributor

mergify bot commented Apr 18, 2024

refresh

✅ Pull request refreshed

mergify bot added a commit that referenced this pull request Apr 18, 2024
@mergify
Merge of #8383
9bef58e
@mergify mergify bot mentioned this pull request Apr 18, 2024
Closed
@mergify mergify bot merged commit f8c1539 into main Apr 19, 2024
189 checks passed
@mergify mergify bot deleted the activ-heights-param-impl branch April 19, 2024 01:20
@arya2 arya2 added this to the Regtest Network support milestone Apr 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@upbqdn upbqdn upbqdn approved these changes

Assignees

@arya2 arya2

Labels
A-consensus Area: Consensus rule updates A-network Area: Network protocol updates or fixes C-enhancement Category: This is an improvement C-feature Category: New features C-testing Category: These are tests I-usability Zebra is hard to understand or use NU-6 Network Upgrade: NU6 specific tasks P-Medium ⚡
Projects
None yet
Milestone
No milestone
2 participants
@arya2 @upbqdn