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

change(rpc): Modify getblocksubsidy for NU6 #8742

Merged
merged 16 commits into from
Aug 15, 2024
Merged

change(rpc): Modify getblocksubsidy for NU6 #8742

merged 16 commits into from
Aug 15, 2024

Conversation

oxarbitrage
Copy link
Contributor

@oxarbitrage oxarbitrage commented Aug 5, 2024 •
edited by arya2
Loading

Motivation

We need to make changes to getblocksubsidy to support lockbox streams and additional total fields.

Outputs of this rpc methods should be the same as implemented in zcash/zcash#6912

Close #8702

Solution

Made modifications that were discussed in the ticket and in the zcash PR, this is a draft as there are still open questions.

Also blocked in having testnet activation height.

Tests

Snapshot tests.

Follow-up Work

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 added NU-6 Network Upgrade: NU6 specific tasks A-rpc Area: Remote Procedure Call interfaces P-Medium ⚡ labels Aug 5, 2024
oxarbitrage
oxarbitrage commented Aug 5, 2024
View reviewed changes
Copy link
Contributor Author

@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.

Some open questions

zebra-rpc/src/methods/get_block_template_rpcs.rs Outdated Show resolved Hide resolved
...rpc/src/methods/tests/snapshot/snapshots/get_block_subsidy_future_nu6_height@testnet_10.snap Outdated Show resolved Hide resolved
...-rpc/src/methods/tests/snapshot/snapshots/get_block_subsidy_excessive_height@testnet_10.snap Outdated Show resolved Hide resolved
@oxarbitrage
Copy link
Contributor Author

For the test failures i think the best will be try to merge zcash/librustzcash#1454 and then upgrade zcash_primitives to main in the context of #8713

Another option is to point zcash_primitives to zcash/librustzcash#1454 instead of main.

Last, we could add flags to our tests temporally to enable/disable.

@mpguerra
Copy link
Contributor

mpguerra commented Aug 6, 2024

For the test failures i think the best will be try to merge zcash/librustzcash#1454 and then upgrade zcash_primitives to main in the context of #8713

We should be able to do this now

@oxarbitrage
Copy link
Contributor Author

oxarbitrage commented Aug 14, 2024 •
edited
Loading

I made some manual testing of this PR to compare the getblocksubsidy RPC method between our implementation and zcashd. Here are some of the findings:

  1. Block 1:

    Command: zebra-utils/zcash-rpc-diff 28232 getblocksubsidy 1
    1

    Zebra expects the block subsidy after the first halving, which is a known and previously accepted difference.

  2. NU5 Activation:

    Command: zebra-utils/zcash-rpc-diff 28232 getblocksubsidy 1842420
    2

    At NU5 activation, both implementations produce the same output. The only variation is in the number of decimals, which has also been previously accepted.

  3. NU6 Activation:

    Command: zebra-utils/zcash-rpc-diff 28232 getblocksubsidy 2942000
    3

    At NU6, the differences align with expectations due to the PR's implementation of activation height in zebrad, as compared to the zcashd implementation, which does not have an activation height.

  4. Post-NU6:

    Command: zebra-utils/zcash-rpc-diff 28232 getblocksubsidy 2942001
    4

    As expected, there are still differences immediately following NU6.

  5. Post-NU6 Period:

    Command: zebra-utils/zcash-rpc-diff 28232 getblocksubsidy 4000000
    5

    Both implementations are the same once the NU6 funding period has ended.

@arya2 arya2 mentioned this pull request Aug 15, 2024
Merged
arya2
arya2 reviewed Aug 15, 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 looks good, the only blocker to merging this into main seems to be either removing the Testnet activation height or waiting for it to be specified. I opened a suggestion PR (#8766) that removes it.

@arya2
Suggestion for "change(rpc): Modify getblocksubsidy for NU6" (#8766)
1a873f5 
* refactors get_block_subsidy RPC method

* replaces a `Default` impl with derived impls

* removes NU6 testnet activation height

* updates snapshot test to use a configured Testnet for the post-NU6 getblocksubsidy output

* fixes snapshot test

* fixes snapshot

* Add a snapshot of getblockchaininfo with a future nu6 height
@arya2 arya2 marked this pull request as ready for review August 15, 2024 21:31
@arya2 arya2 requested review from a team as code owners August 15, 2024 21:31
@arya2 arya2 requested review from upbqdn and removed request for a team August 15, 2024 21:31
mergify bot added a commit that referenced this pull request Aug 15, 2024
@mergify
Merge of #8742
842b056
@mergify mergify bot mentioned this pull request Aug 15, 2024
Closed
@mergify mergify bot merged commit 11b0833 into main Aug 15, 2024
157 of 158 checks passed
@mergify mergify bot deleted the block-subsidy-nu6 branch August 15, 2024 22:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@arya2 arya2 arya2 approved these changes

@upbqdn upbqdn Awaiting requested review from upbqdn upbqdn is a code owner automatically assigned from ZcashFoundation/network-reviewers

Assignees
No one assigned
Labels
A-rpc Area: Remote Procedure Call interfaces NU-6 Network Upgrade: NU6 specific tasks P-Medium ⚡
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

change(rpc): Ensure mining pools will accept responses to getblocksubsidy
3 participants
@oxarbitrage @mpguerra @arya2