Block Subsidy and Founders Reward Amounts

[oxarbitrage]

This PR calculates:

• block subsidy amounts
• founders reward amounts

And checks:

• founders reward amounts

We can't check the block subsidy yet, because we don't calculate miner fees in this PR.

Part of #338

[yaahc]

Did some initial review, only about half of the PR but I think I left enough comments for you to make a bunch more progress before my next more comprehensive review.

[oxarbitrage]

Thanks @yaahc , that was great, specially the combinator suggestions that i have big troubles to implement on my own :) I will take care of all the changes.

[teor2345]

Hi @oxarbitrage, just wondering if you'd like a detailed review here?

It seems like the Canopy Funding Streams implementation isn't finished yet - do you want to focus on the Founders Reward in this PR, and do Funding Streams in a separate PR?

I also noticed a few things that could improve the code:

• replace activation height constants with the NetworkUpgrade::activation_height function, so the code works on mainnet and testnet
• use Amount<NonNegative>, rather than u32 or f32, so that all the calculations are accurate (and don't overflow)
  • let me know if you need help converting from f32 to Amount
• write a short doc comment for each function and constant, explaining what it does, and how it is used
  • for most functions, you can just write one sentence that explains what it does with each argument

[oxarbitrage]

Thanks @teor2345 . Yea, we can do funding streams in a separated PR so we can merge faster. Feel free to make a detailed review, i will try to make the changes tomorrow. In addition to what you mentioned this PR needs a rebase now that #1057 is merged. I removed the draft state.

[oxarbitrage]

@teor2345 i was thinking on adding the formulas in latex from https://zips.z.cash/protocol/protocol.pdf#subsidies into the comments of the subsidy functions. let me know what do you think.

In order to do this i was expecting #832 to work but in some small test i made it seems the output documents are not rendering the latex content. Just need to set env variable RUSTDOCFLAGS="--html-in-header katex-header.html" to make the render work locally.

[teor2345]

There's a lot of good work here - there are so many block subsidy rules to cover!

During my review, I tried to focus on the correctness of the code.

(the table was wrong, so I moved it to a later comment, and corrected it)

Here are some other things we can do to make sure the code is correct, and make it easier to read:

• use the appropriate types: block::Height and Amount<NonNegative>
• explain what each complex calculation does, and why it is accurate - this will be a lot easier once the calculations are split into separate functions

Please also choose a consistent name for the subsidy modules. At the moment, we have:

• block::subsidies
• parameters::subsidy

In this case, the standard term is subsidy. (If there isn't a standard term, I try to use the singular name. That is, subsidy rather than subsidies.)

[oxarbitrage]

In regards to the use of Amount<NonNegative> in constants and function return values i see some problems that i will like to be clarified before implementing. In addition to Add and Sub there are several functions or part of them that will need Div and Mul. To avoid conversions back and forth one option can be to add this operators to the Amount type. For the functions that return Amount<NonNegative>(and maybe all functions) we can wrap in Result<Amount<NonNegative>, Err> so we can have ? at hand.

Let me know what do you think about that @teor2345 as i maybe have something missing. Thanks.

[yaahc]

In regards to the use of Amount<NonNegative> in constants and function return values i see some problems that i will like to be clarified before implementing. In addition to Add and Sub there are several functions or part of them that will need Div and Mul. To avoid conversions back and forth one option can be to add this operators to the Amount type. For the functions that return Amount<NonNegative>(and maybe all functions) we can wrap in Result<Amount<NonNegative>, Err> so we can have ? at hand.

Let me know what do you think about that @teor2345 as i maybe have something missing. Thanks.

Adding Div/Mul and returning Result is the right way to go imo.

[teor2345]

We try to format errors as "error kind: conditions that caused the error".

The checks in the function are incomplete, so it is hard to write a good error description. Here is an error with all the checks that the function should do. As you can see, the error is very long.

So we should split up the checks for:

• the sum of the coinbase outputs, and
• the founders reward.

Since we want to check the founders reward for blocks 1..Canopy, but always check the coinbase output sum, these checks should be in different functions.

Suggested change

	Err("validation of block subsidy rules inside coinbase transaction failed")?
	Err("invalid coinbase transaction: the sum of the coinbase transaction outputs must be less than or equal to the block subsidy plus transaction fees, and the exact founders reward value must be sent as a single output to the correct address")?

[teor2345]

We can't do the sum of coinbase outputs check yet, so let's just check the founders reward, and error if it fails.

Then we can add a TODO and Ok(()) at the end of the function

[teor2345]

In regards to the use of Amount<NonNegative> in constants and function return values i see some problems that i will like to be clarified before implementing. In addition to Add and Sub there are several functions or part of them that will need Div and Mul. To avoid conversions back and forth one option can be to add this operators to the Amount type. For the functions that return Amount<NonNegative>(and maybe all functions) we can wrap in Result<Amount<NonNegative>, Err> so we can have ? at hand.

Adding Div/Mul and returning Result is the right way to go imo.

How should we handle inexact divisions?
(That is, a division that has a remainder.)

I'd like to make them errors, so that we explicitly handle remainders in every calculation.
We can use expect for calculations that will never have a remainder, and match otherwise.

Here's the list of operators I think we'll want for amount:

• Add<Amount, Rhs=Amount, Output=Result<Amount>>
• Sub<Amount, Rhs=Amount, Output=Result<Amount>>
• Mul<Amount, Rhs=u64, Output=Result<Amount>>
• Mul<u64, Rhs=Amount, Output=Result<Amount>>
  • Implement using amount * self
• Div<Amount, Rhs=u64, Output=Result<Amount, DivisionError>>
  • with enum DivisionError { DivideByZeroError, InexactDivisionError(remainder: Amount) }
  • overflow and underflow can't happen when dividing by a positive integer

We should also implement the corresponding ...Assign traits, using the non-...Assign trait.

Add, Sub and Mul can have these errors:

• overflow: result greater than MAX_MONEY
• underflow: result less than -MAX_MONEY or 0, depending on the Amount type

Things we might want to implement - but let's wait and see if we need them:

• Rem<Amount, Rhs=u64, Output=Result<Amount>>
  • Implement using self - ((self / rhs) * rhs)
• Neg<Amount, Output=Result<Amount>>
  • Implement using Amount(0) - self

Things we don't want to implement, because they are probably coding errors:

• multiplying two amounts
• multiplying an amount by a negative integer
• dividing two amounts (to get an integer)
• dividing an amount by a negative integer

[teor2345]

The rules table I originally posted was incomplete and incorrect. Here's my second attempt:

Here is my understanding of the block subsidy rules from the spec - rules that we don't need to handle are in strikethrough:

Height	Slow Start Multiplier	Target Spacing	Distribution Rules
Genesis	(no coinbase)	(none)	(none)
1..Slow Start Shift	height	150 seconds	Founders Reward
Slow Start Shift..Slow Start Interval	height + 1	150 seconds	Founders Reward
Slow Start Interval Sapling..Blossom		150 seconds	Founders Reward
Blossom..Canopy		75 seconds	Founders Reward
Canopy..Second Halving		75 seconds	Funding Streams
Second Halving..		75 seconds	Only Miners

This rules summary table can go in the module doc comment of the block subsidy module.

In this PR, we'll implement the rules from:

• Sapling
• Blossom
• Heartwood

Here's how we'll handle the rules from the other network upgrades:

• the Genesis, BeforeOverwinter, and Overwinter blocks are verified by the CheckpointVerifier, so we can:
  • delete any code for those network upgrades
  • panic! if we are asked to verify a block from those network upgrades
• delete the post-Canopy code from this PR, and move it to another PR

I think it would be helpful to have functions for each different set of rules, and group those functions into modules. Then the top-level subsidy module can select the appropriate rules using a match statement.

For example, we could have modules for:

• general subsidy: maximum subsidy, halvings
• slow start subsidy modifiers (before and after shift) - not required, delete this code
• target spacing subsidy modifiers
• founders reward distribution
• funding streams distribution - delete code from this PR, and move to next PR

[teor2345]

Hey @oxarbitrage let me know when you're ready for another review.

Can I do the review before you rebase on the latest main branch?
It will be easier to see the new changes.

[teor2345]

Looking good!

I tried to suggest some type and calculation fixes, let me know if they don't work out.

[teor2345]

Just some updates based on our conversations over the last day

[teor2345]

We can't do the sum of coinbase outputs check yet, so let's just check the founders reward, and error if it fails.

Then we can add a TODO and Ok(()) at the end of the function

[teor2345]

@oxarbitrage now that we have a stable design for Mul and Div, do you also want to implement:

• MulAssign<u64> for Amount<NonNegative>
• DivAssign<u64> for Amount<NonNegative>

[oxarbitrage]

I think we are going to be comparing amounts more sooner than later so added the operators to do it at 7b0f166

[oxarbitrage]

@oxarbitrage now that we have a stable design for Mul and Div, do you also want to implement:

MulAssign for Amount
DivAssign for Amount

I implemented in 453ad1f however having some issues to make them safe. I cant return a Result in those operators so i am silently returning the Amount unmodified if overflow or division by zero. Probably some workaround is needed but i was not able to find any that convince me yet.

[teor2345]

@oxarbitrage now that we have a stable design for Mul and Div, do you also want to implement:
MulAssign for Amount
DivAssign for Amount

I implemented in 453ad1f however having some issues to make them safe. I cant return a Result in those operators so i am silently returning the Amount unmodified if overflow or division by zero. Probably some workaround is needed but i was not able to find any that convince me yet.

Try copying the implementations from:
https://github.com/ZcashFoundation/zebra/blob/main/zebra-chain/src/amount.rs#L76

And changing the types.

I approve the bits I didn't write

[teor2345]

Hey @oxarbitrage, I just realised that we're missing failure tests. Let's write a test for each error:

• NoCoinbase
• FoundersRewardNotFound

It should be easy to make these tests using generate, or by deleting transactions/outputs from valid blocks. Can you do that today?

It would also be good to make sure that we allow duplicate outputs, as long as at least one output passes - I'll put that on the tracking issue.

I also:

• did some fixes for @yaahc's review in Block subsidy: matches to if-elses oxarbitrage/zebra#30
• added shielded coinbase blocks in Shielded coinbase test blocks #1116
• tested that this PR works with the shielded coinbase blocks

[teor2345]

Oops, I think we missed this PR - we still need a review from someone who isn't me or @oxarbitrage.

[teor2345]

I made a mistake with testnet funding stream activation, I'll fix it soon.

I fixed the bug that I caused

[teor2345]

I think this is good to go, but I'd like to get someone else to look at the bits I wrote.

[oxarbitrage]

The last fix looks good, i think i already checked the rest. We should merge this one and IMHO, continue with the block subsidy design(#1129) and implement the rest of it in separated PRs.

[yaahc]

LGTM