add(consensus/network): Add an empty `Parameters` struct in `Network::Testnet` #8368

arya2 · 2024-03-20T15:44:26Z

Motivation

We want to add configurable network parameters to Network for supporting Regtest and custom Testnets to be used in integration tests.

Closes #7968.

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.

Complex Code or Requirements

The db format for HistoryTrees and transparent::Addresses are the same across any test networks, this could mean that a Regtest cache or a custom Testnet cache could be used on the default Testnet. We may want to follow up this PR with a change that ensures Zebra panics if it's configured to run on the default public testnet with a Regtest cache, or we may want to make that change in this PR.

I'd like special attention to changes in:

The db format upgrade code where a network == Testnet condition was replaced with network.is_a_test_network()
Tests where Network::iter() was used instead of running with Mainnet and Testnet being applicable to Regtest

Solution

Adds a NetworkParameters struct and includes it in Network::Testnet
Adds a new_default_testnet() method for creating the default public Testnet
Updates a condition in the db format upgrade to be true for any test network
Updates the key types in:
- FUNDING_STREAM_HEIGHT_RANGES,
- FUNDING_STREAM_ADDRESSES, and
- INITIAL_MIN_NETWORK_PROTOCOL_VERSION
Updates production code matching on Testnet to return appropriate values
Adds TODOs to production code where changes are needed when adding new fields
Uses Network::iter() in tests that run equivalent code for both Mainnet and Testnet
Replaces instances of the Testnet constructor with Network::new_default_testnet()
Changes type of network field on transparent::Address and HistoryTreeParts to NetworkKind

Testing

Network::iter() was used in most tests that check both Mainnet and Testnet so they'll cover any new networks we return from that function.

This PR may still need a test for the custom deserialization of Network or other changes.

Review

Anyone can review.

This PR may be easier to review by commit:

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

Start adding fields to NetworkParameters, starting with activation_heights (Add a field for the network upgrade activation heights to TestnetParameters #7970)

zebra-chain/src/parameters/network.rs

zebra-chain/src/primitives/address.rs

zebra-chain/src/parameters/network.rs

zebra-chain/src/primitives/zcash_primitives.rs

zebra-chain/src/transparent/address.rs

zebra-consensus/src/checkpoint/list.rs

zebra-network/src/config.rs

zebra-consensus/src/parameters/subsidy.rs

zebra-network/src/config.rs

zebra-network/src/protocol/external/types.rs

zebra-chain/src/primitives/address.rs

idky137

Should we have new_default_regtest and is_a_regtest methods similar to testnet? This would be useful for wallets to use in testing and simplify migration. They could also be used where we need to distinguish between public_testnet and regtest.

arya2 · 2024-03-27T14:36:44Z

Should we have new_default_regtest and is_a_regtest methods similar to testnet?

Yes, we should add a is_regtest() method, but not in this PR, we want to minimize the changes here so it's easy to review, so we're only adding an empty struct to Testnet where we can start adding fields that will be needed for Regtest.

zebra-state/src/service/finalized_state/disk_format/chain.rs

…` module

arya2 · 2024-04-11T18:13:04Z

It's a bit unfortunate we need to overload the word "network" since we also have a crate called zebra-network

Yep. It caused some indecisiveness when I was naming the PR and choosing labels, since the PR is related to which Zcash network the node is in, but I think "consensus" actually fits better, I'll update the PR title to include both.

Running a full sync test here:
https://github.com/ZcashFoundation/zebra/actions/runs/8651283991

zebra-chain/src/primitives/zcash_note_encryption.rs

zebra-rpc/src/methods/get_block_template_rpcs.rs

upbqdn · 2024-04-12T17:09:29Z

I changed NetworkParameters to Parameters in the PR name.

arya2 · 2024-04-12T18:12:49Z

Added a do-not-merge label to wait on the full sync test to finish.

arya2 · 2024-04-15T14:26:09Z

@Mergifyio refresh

mergify · 2024-04-15T14:26:13Z

refresh

✅ Pull request refreshed

arya2 · 2024-04-15T18:14:14Z

@Mergifyio refresh

mergify · 2024-04-15T18:14:36Z

refresh

✅ Pull request refreshed

arya2 · 2024-04-17T01:04:51Z

@Mergifyio refresh

mergify · 2024-04-17T01:04:54Z

refresh

✅ Pull request refreshed

arya2 added A-consensus Area: Consensus rule updates 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 P-Medium ⚡ labels Mar 20, 2024

arya2 self-assigned this Mar 20, 2024

github-actions bot added the C-trivial Category: A trivial change that is not worth mentioning in the CHANGELOG label Mar 20, 2024

arya2 added this to the Regtest Network support milestone Mar 20, 2024

idky137 reviewed Mar 21, 2024

View reviewed changes

zebra-chain/src/parameters/network.rs Outdated Show resolved Hide resolved

arya2 force-pushed the network-params branch 2 times, most recently from dbf3296 to c342e5f Compare March 22, 2024 01:27

arya2 marked this pull request as ready for review March 22, 2024 01:53

arya2 requested review from a team as code owners March 22, 2024 01:53

arya2 requested review from upbqdn and removed request for a team March 22, 2024 01:53

arya2 added the extra-reviews This PR needs at least 2 reviews to merge label Mar 22, 2024

arya2 requested a review from oxarbitrage March 22, 2024 01:54

arya2 commented Mar 26, 2024

View reviewed changes

zebra-chain/src/transparent/address.rs Outdated Show resolved Hide resolved

zebra-consensus/src/checkpoint/list.rs Outdated Show resolved Hide resolved

arya2 commented Mar 26, 2024

View reviewed changes

zebra-consensus/src/checkpoint/list.rs Outdated Show resolved Hide resolved

zebra-network/src/config.rs Outdated Show resolved Hide resolved

zebra-consensus/src/parameters/subsidy.rs Outdated Show resolved Hide resolved

arya2 commented Mar 27, 2024

View reviewed changes

zebra-network/src/config.rs Outdated Show resolved Hide resolved

zebra-network/src/config.rs Show resolved Hide resolved

zebra-network/src/protocol/external/types.rs Show resolved Hide resolved

idky137 reviewed Mar 27, 2024

View reviewed changes

zebra-chain/src/primitives/address.rs Outdated Show resolved Hide resolved

idky137 reviewed Mar 27, 2024

View reviewed changes

arya2 commented Mar 27, 2024

View reviewed changes

zebra-state/src/service/finalized_state/disk_format/chain.rs Show resolved Hide resolved

minor cleanup and rename

9db4762

arya2 added 2 commits April 11, 2024 13:56

Rename NetworkParameters to Parameters and move it a new `testnet…

365b58e

…` module

Merge branch 'main' into network-params

eca7f6b

arya2 changed the title ~~add(network): Add an empty NetworkParameters struct in Network::Testnet~~ add(consensus/network): Add an empty NetworkParameters struct in Network::Testnet Apr 11, 2024

arya2 requested a review from upbqdn April 11, 2024 20:49

upbqdn changed the title ~~add(consensus/network): Add an empty NetworkParameters struct in Network::Testnet~~ add(consensus/network): Add an empty Parameters struct in Network::Testnet Apr 12, 2024

upbqdn approved these changes Apr 12, 2024

View reviewed changes

zebra-chain/src/primitives/zcash_note_encryption.rs Show resolved Hide resolved

zebra-rpc/src/methods/get_block_template_rpcs.rs Show resolved Hide resolved

arya2 added the do-not-merge Tells Mergify not to merge this PR label Apr 12, 2024

arya2 removed the C-trivial Category: A trivial change that is not worth mentioning in the CHANGELOG label Apr 12, 2024

This was referenced Apr 12, 2024

add(consensus): Adds Sapling HRPs as fields on testnet::Parameters #8398

Merged

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

Merged

mpguerra removed the do-not-merge Tells Mergify not to merge this PR label Apr 15, 2024

arya2 removed the extra-reviews This PR needs at least 2 reviews to merge label Apr 15, 2024

mergify bot added a commit that referenced this pull request Apr 15, 2024

Merge of #8368

a4c0813

mergify bot mentioned this pull request Apr 15, 2024

merge queue: embarking main (1534442) and #8368 together #8403

Closed

arya2 added do-not-merge Tells Mergify not to merge this PR and removed do-not-merge Tells Mergify not to merge this PR labels Apr 15, 2024

mergify bot added a commit that referenced this pull request Apr 17, 2024

Merge of #8368

71d0c14

mergify bot added a commit that referenced this pull request Apr 17, 2024

Merge of #8368

79d1346

mergify bot mentioned this pull request Apr 17, 2024

merge queue: embarking main (1498f8e) and [#8402 + #8368] together #8405

Closed

mergify bot merged commit 16a39f8 into main Apr 17, 2024
211 checks passed

mergify bot deleted the network-params branch April 17, 2024 02:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add(consensus/network): Add an empty `Parameters` struct in `Network::Testnet` #8368

add(consensus/network): Add an empty `Parameters` struct in `Network::Testnet` #8368

arya2 commented Mar 20, 2024 •

edited

Loading

idky137 left a comment

arya2 commented Mar 27, 2024

arya2 commented Apr 11, 2024

upbqdn commented Apr 12, 2024

arya2 commented Apr 12, 2024

arya2 commented Apr 15, 2024

mergify bot commented Apr 15, 2024

arya2 commented Apr 15, 2024

mergify bot commented Apr 15, 2024

arya2 commented Apr 17, 2024

mergify bot commented Apr 17, 2024

add(consensus/network): Add an empty Parameters struct in Network::Testnet #8368

add(consensus/network): Add an empty Parameters struct in Network::Testnet #8368

Conversation

arya2 commented Mar 20, 2024 • edited Loading

Motivation

PR Author Checklist

Check before marking the PR as ready for review:

For significant changes:

Complex Code or Requirements

Solution

Testing

Review

Reviewer Checklist

Follow Up Work

idky137 left a comment

Choose a reason for hiding this comment

arya2 commented Mar 27, 2024

arya2 commented Apr 11, 2024

upbqdn commented Apr 12, 2024

arya2 commented Apr 12, 2024

arya2 commented Apr 15, 2024

mergify bot commented Apr 15, 2024

✅ Pull request refreshed

arya2 commented Apr 15, 2024

mergify bot commented Apr 15, 2024

✅ Pull request refreshed

arya2 commented Apr 17, 2024

mergify bot commented Apr 17, 2024

✅ Pull request refreshed

add(consensus/network): Add an empty `Parameters` struct in `Network::Testnet` #8368

add(consensus/network): Add an empty `Parameters` struct in `Network::Testnet` #8368

arya2 commented Mar 20, 2024 •

edited

Loading