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

change(network-params): Configurable Testnet funding streams #8718

Merged
merged 9 commits into from
Jul 29, 2024

Conversation

arya2
Copy link
Contributor

@arya2 arya2 commented Jul 23, 2024

Motivation

We want configurable funding streams to test consensus rule changes in NU6.

Closes #8367.

Depends-On: #8723.

Solution

  • Moves subsidy.rs from zebra-consensus to zebra-chain
  • Moves funding_stream_address_period() fn to zebra-chain
  • Refactors funding stream information into structs
  • Splits funding streams into pre/post NU6 funding streams
    • Adds expected height ranges for post-NU6 funding streams with empty recipients list
  • Adds a funding_streams field on testnet::Parameters

Related changes:

  • Returns early from is_regtest() after checking the network magic
  • Moves funding stream receiver names to a method
  • Adds missing #[serde(deny_unknown_fields)] on Testnet parameters struct in zebra-network config deserialize impl

Tests

This PR adds a vector test for the constraints on ParametersBuilder::with_funding_streams() setter methods and updates the v1.9.0 stored config.

PR Author's Checklist

  • The PR name will make sense to users.
  • The PR provides a CHANGELOG summary.
  • The solution is tested.
  • The documentation is up to date.
  • The PR has a priority label.

Follow Up Work

Move the zebra_consensus::block::subsidy module to zebra-chain.

PR Reviewer's Checklist

  • The PR Author's checklist is complete.
  • The PR resolves the issue.

@arya2 arya2 requested a review from oxarbitrage July 23, 2024 16:47
@github-actions github-actions bot added the C-trivial Category: A trivial change that is not worth mentioning in the CHANGELOG label Jul 23, 2024
@arya2 arya2 self-assigned this Jul 23, 2024
@arya2 arya2 marked this pull request as ready for review July 24, 2024 00:39
@arya2 arya2 requested review from a team as code owners July 24, 2024 00:39
@arya2 arya2 added NU-6 Network Upgrade: NU6 specific tasks C-testing Category: These are tests P-Medium ⚡ A-consensus Area: Consensus rule updates and removed C-trivial Category: A trivial change that is not worth mentioning in the CHANGELOG labels Jul 24, 2024
@arya2 arya2 requested a review from upbqdn July 24, 2024 00:49
@arya2 arya2 force-pushed the config-testnet-funding-streams branch from 077acc2 to 82ccd0b Compare July 24, 2024 00:55
@github-actions github-actions bot added the C-trivial Category: A trivial change that is not worth mentioning in the CHANGELOG label Jul 24, 2024
@arya2 arya2 force-pushed the config-testnet-funding-streams branch 2 times, most recently from 5f17a50 to 9a9464c Compare July 24, 2024 01:05
@arya2 arya2 added the extra-reviews This PR needs at least 2 reviews to merge label Jul 24, 2024
oxarbitrage
oxarbitrage previously approved these changes Jul 24, 2024
Copy link
Contributor

@oxarbitrage oxarbitrage 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, i think we can clarify some of the block numbers maybe adding them as constants so we can change them more easy.

oxarbitrage
oxarbitrage previously approved these changes Jul 24, 2024
Copy link
Contributor

@oxarbitrage oxarbitrage left a comment

Choose a reason for hiding this comment

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

Thanks for the updates.

@arya2 arya2 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 Jul 25, 2024
mergify bot added a commit that referenced this pull request Jul 25, 2024
Copy link
Collaborator

@conradoplg conradoplg left a comment

Choose a reason for hiding this comment

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

Left some comments, also, are the TODOs added supposed to be addressed in a later PR or in this one still?

@arya2
Copy link
Contributor Author

arya2 commented Jul 25, 2024

Left some comments, also, are the TODOs added supposed to be addressed in a later PR or in this one still?

They're all meant to be addressed in a later PR, the ones related to referencing the final versions of draft ZIPs are meant to be addressed when those draft ZIPs are finalized, the rest are addressed in #8694.

…tructs, splits them into pre/post NU6 funding streams, and adds them as a field on `testnet::Parameters`
…onversion logic with constraints.

Minor refactors
…red Testnets, but that being okay since configured testnet parameters are checked when they're being built
@arya2 arya2 added the extra-reviews This PR needs at least 2 reviews to merge label Jul 25, 2024
@arya2 arya2 force-pushed the config-testnet-funding-streams branch from 852d634 to b03e6a2 Compare July 25, 2024 21:05
@github-actions github-actions bot added the C-trivial Category: A trivial change that is not worth mentioning in the CHANGELOG label Jul 25, 2024
@arya2 arya2 changed the base branch from main to fix-new-clippy-lints July 25, 2024 21:06
@arya2 arya2 force-pushed the config-testnet-funding-streams branch from b03e6a2 to 7d7f606 Compare July 25, 2024 21:14
Base automatically changed from fix-new-clippy-lints to main July 26, 2024 14:29
@arya2 arya2 removed the extra-reviews This PR needs at least 2 reviews to merge label Jul 26, 2024
@mergify mergify bot merged commit 988dd55 into main Jul 29, 2024
192 checks passed
@mergify mergify bot deleted the config-testnet-funding-streams branch July 29, 2024 16:15
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 C-testing Category: These are tests C-trivial Category: A trivial change that is not worth mentioning in the CHANGELOG NU-6 Network Upgrade: NU6 specific tasks P-Medium ⚡
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add protocol version information to fields on NetworkParameters
5 participants