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

feat(regtest): Add regtest halving interval and port test #8888

Merged
merged 17 commits into from
Oct 10, 2024
Merged

feat(regtest): Add regtest halving interval and port test #8888

merged 17 commits into from
Oct 10, 2024

Conversation

oxarbitrage
Copy link
Contributor

Motivation

We want to be able to configure the halving interval when we are in custom testnet or use the fixed default in regtest.

Close #8880

Solution

  • Added FIRST_HALVING_REGTEST at height 144 to match zcashd regtest mode.
  • Added configuration option halving_interval for custom testnets.
  • Ported the nuparams.py tests from zcashd
  • Fixed inconsistency between Nu6 and NU6 in rpc outputs.

Tests

./qa/pull-tester/rpc-tests.py nuparams

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.

PR Reviewer's Checklist

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

@oxarbitrage oxarbitrage requested review from a team as code owners September 23, 2024 23:09
@oxarbitrage oxarbitrage requested review from arya2 and removed request for a team September 23, 2024 23:09
@github-actions github-actions bot added C-feature Category: New features C-trivial Category: A trivial change that is not worth mentioning in the CHANGELOG labels Sep 23, 2024
arya2
arya2 reviewed Sep 24, 2024
View reviewed changes
zebra-network/src/config.rs Outdated Show resolved Hide resolved
@oxarbitrage oxarbitrage added C-testing Category: These are tests A-compatibility Area: Compatibility with other nodes or wallets, or standard rules P-Medium ⚡ and removed C-trivial Category: A trivial change that is not worth mentioning in the CHANGELOG labels Sep 26, 2024
arya2
arya2 reviewed Sep 26, 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 is looking good, the only blocker is an update to the height_for_first_halving() method so it returns FIRST_HALVING_TESTNET on custom testnets, or ideally the correct value. I'll try writing a halving_for_height() function so we can use the correct value there.

Update: FUNDING_STREAM_ADDRESS_CHANGE_INTERVAL and the funding_stream_address_period() are also using the constant post blossom halving interval.

@arya2 arya2 mentioned this pull request Sep 26, 2024
Merged
@arya2
Suggestion for "feat(regtest): Add regtest halving interval and port …
120ae74 
…test" (#8894)

* adds `height_for_halving_index()` and `num_halvings()` fns

* avoid unnecessary panic

* avoid using constant pre/post blossom halving intervals in num_halvings()
@github-actions github-actions bot added the C-trivial Category: A trivial change that is not worth mentioning in the CHANGELOG label Sep 27, 2024
@oxarbitrage oxarbitrage requested a review from arya2 October 3, 2024 12:01
@oxarbitrage oxarbitrage removed the C-trivial Category: A trivial change that is not worth mentioning in the CHANGELOG label Oct 3, 2024
arya2
arya2 reviewed Oct 4, 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.

This is looking great, the only potential blocker is an update to TestnetParameterSubsidyImpl to account for the halving interval and Blossom activation height when validating configured funding streams.

zebra-chain/src/parameters/network/subsidy.rs Show resolved Hide resolved
zebra-chain/src/parameters/network/testnet.rs Outdated Show resolved Hide resolved
zebra-chain/src/parameters/network/subsidy.rs Outdated Show resolved Hide resolved
zebra-chain/src/parameters/network/subsidy.rs Outdated Show resolved Hide resolved
@github-actions github-actions bot added the C-trivial Category: A trivial change that is not worth mentioning in the CHANGELOG label Oct 7, 2024
This was referenced Oct 10, 2024
Merged
Open
@arya2
Updates new_regtest() method to return a Testnet without funding st…
dea0ce4 
…reams, updates funding stream setter methods to set a flag indicating that parameters affecting the funding stream address period should be locked, updates the setter methods for parameters that affect the funding stream address period to panic if those parameters should be locked. (#8921)
arya2
arya2 approved these changes Oct 10, 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!

mergify bot added a commit that referenced this pull request Oct 10, 2024
@mergify
Merge of #8888
7066f79
@mergify mergify bot mentioned this pull request Oct 10, 2024
Closed
@mergify mergify bot merged commit f2e7bc9 into main Oct 10, 2024
205 checks passed
@mergify mergify bot deleted the regtest-halving-interval branch October 10, 2024 18:26
@arya2 arya2 mentioned this pull request Oct 10, 2024
Merged
dmidem pushed a commit to QED-it/zebra that referenced this pull request Oct 29, 2024
@oxarbitrage @arya2 @dmidem
feat(regtest): Add regtest halving interval and port test (ZcashFound…
e37072a 
…ation#8888)

* add halving interval to regtest and to custom testnet

* add nuparams.py rpc test

* fix inconsistency in nu6 name in rpc methods

* rename `halving_interval` to `pre_blossom_halving_interval` in the config

* make fixes

* Suggestion for "feat(regtest): Add regtest halving interval and port test" (ZcashFoundation#8894)

* adds `height_for_halving_index()` and `num_halvings()` fns

* avoid unnecessary panic

* avoid using constant pre/post blossom halving intervals in num_halvings()

* make regtest and testnet constant more private

* move `height_for_halving_index`

* fmt

* add a `funding_stream_address_change_interval` method

* add checked operations to `height_for_halving_index` fn

* add post_blossom interval as paramneters + other refactors

* rename function

* fix docs

* move constant

* Updates `new_regtest()` method to return a Testnet without funding streams, updates funding stream setter methods to set a flag indicating that parameters affecting the funding stream address period should be locked, updates the setter methods for parameters that affect the funding stream address period to panic if those parameters should be locked. (ZcashFoundation#8921)

---------

Co-authored-by: Arya <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mpguerra mpguerra mpguerra left review comments

@arya2 arya2 arya2 approved these changes

Assignees
No one assigned
Labels
A-compatibility Area: Compatibility with other nodes or wallets, or standard rules C-feature Category: New features C-testing Category: These are tests C-trivial Category: A trivial change that is not worth mentioning in the CHANGELOG P-Medium ⚡
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Make the halving interval configurable for custom networks and regtests
3 participants
@oxarbitrage @mpguerra @arya2