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

Create function for post-blossom height adjustments #1311

Closed
oxarbitrage opened this issue Nov 17, 2020 · 5 comments
Closed

Create function for post-blossom height adjustments #1311

oxarbitrage opened this issue Nov 17, 2020 · 5 comments
Labels
C-enhancement Category: This is an improvement

Comments

@oxarbitrage
Copy link
Contributor

Is your feature request related to a problem? Please describe.

It feels like we do post-blossom height adjustments in a few different parts of the code.

Is there a way to abstract them into a new function?

#1170 (comment)

Describe the solution you'd like

Create a function that can be used from at least 2 places.

Describe alternatives you've considered

Leave it as it is.

Additional context

#1170

This is not an urgent task.

@oxarbitrage oxarbitrage added C-enhancement Category: This is an improvement S-needs-triage Status: A bug report needs triage labels Nov 17, 2020
@teor2345 teor2345 added this to the Transaction Validation milestone Nov 17, 2020
@mpguerra mpguerra removed this from the Transaction Validation milestone Jan 5, 2021
@mpguerra mpguerra removed the S-needs-triage Status: A bug report needs triage label Feb 2, 2021
@oxarbitrage
Copy link
Contributor Author

The problem i see with this is that the adjustments made post post blossom are not the same in the 2 places we need to do them.

There is one adjustment we do in halving_divisor():

halving

And the other one in founders_reward_address():

foundersaddress

The are some differences that i cant see how to abstract:

1- The height for pre blossom is scaled in the halving calculation while the height pre blossom for the founders reward is not modified.
2- I cant find any point of coincidence in the post blossom calculations of the 2 spec points, they even use different constants. Maybe my algebra is not good enough or there is something i am not seeing.

@daira
Copy link
Contributor

daira commented Mar 7, 2021

I would strongly suggest to follow the spec as literally as possible here. It took quite some thinking and review to get it right.

@teor2345
Copy link
Contributor

teor2345 commented Mar 8, 2021

If the calculations are different, they should use different functions, which should have function docs describing the differences and pointing to the relevant sections of the spec. Since the existing functions are fairly simple, you could add explanations to them.

Here are the different places that a blossom adjustment happens:
https://github.com/ZcashFoundation/zebra/pull/1170/files#diff-1e411c4dd6dc3ad59094c14b82cba0fdd4fee34f841ede1ab092d87a65ff77f9R51-R54
https://github.com/ZcashFoundation/zebra/pull/1170/files#diff-1e411c4dd6dc3ad59094c14b82cba0fdd4fee34f841ede1ab092d87a65ff77f9R71-R75

let scaled_max_block_subsidy = MAX_BLOCK_SUBSIDY / BLOSSOM_POW_TARGET_SPACING_RATIO;
// in future halvings, this calculation might not be exact
// Amount division is implemented using integer division,
// which truncates (rounds down) the result, as specified
Amount::try_from(scaled_max_block_subsidy / halving_div)

Can you match each one with the relevant part of the spec?
(You only quoted two spec sections above, but there are 3 adjustments.)

Are there any other adjustments?

@mpguerra
Copy link
Contributor

@oxarbitrage do we still need to deal with this now that we are checkpointing on canopy?

@oxarbitrage
Copy link
Contributor Author

In my view we should not do the changes proposed here not because we are checkpointing on canopy but iirc just because is clearer to leave it as it is now as it follows the spec more closely. We can always do this if we need/want but i don't think we need an open ticket. Closing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-enhancement Category: This is an improvement
Projects
None yet
Development

No branches or pull requests

4 participants