ZcashFoundation / zebra

Zcash - Financial Privacy in Rust 🦓
https://zfnd.org/zebra/
Apache License 2.0
408 stars 98 forks source link

Add support for `getblocksubsidy` RPC call #5302

Closed mpguerra closed 1 year ago

mpguerra commented 2 years ago

Motivation

Mining pools use the getblocksubsidy RPC calls.

Mining Pool Usage

s-nomp calls getblocksubsidy from this code:

Requirements

https://zcash.github.io/rpc/getblocksubsidy.html

Height argument

Usage

This RPC is called for the next block height to be mined, before that block is created or validated by the node. The expected response is the subsidy amounts. Transaction fees are not included.

This RPC does not need to support founders reward heights, because all those blocks are mined already.

Design

We need the fixed miner reward and funding stream values for future heights.

Fields:

We can get the funding stream recipient addresses and amounts fromfunding_stream_values(): https://github.com/ZcashFoundation/zebra/blob/dc9da24c8b6cee5c05315f0d77bee52613f75fd3/zebra-consensus/src/block/subsidy/funding_streams.rs#L21-L28

We can get the fixed miner reward from zebra_consensus::subsidy as well.

Error Handling

If the RPC is called for a founders reward block, just return an error.

Testing

Follow-Up Work

Testing:

teor2345 commented 2 years ago

@oxarbitrage the consensus rules allow multiple outputs to be sent to a funding stream recipient, as long as at least one of them is the required amount.

I updated the description, and added a call to funding_stream_values().

teor2345 commented 2 years ago

Also, we only need the founders reward code if this RPC is actually called for those blocks.

teor2345 commented 2 years ago

@mpguerra we won't be able to fully scope this RPC until we get some more information from miners.

It is not called by s-nomp, so I'll ask Dan to follow up.

oxarbitrage commented 2 years ago

Thank you for adding more details @teor2345 . I think the following parts are confusing:

Error Handling

If the RPC is called for a block we don't have yet, just return an error.

But below there seems to be a contradiction:

...

Funding streams for blocks we don't have yet

Use funding_stream_values(), and remove the miner fee field, because we don't know that yet.

I think we should do whatever zcashd does here.

teor2345 commented 2 years ago

Thank you for adding more details @teor2345 . I think the following parts are confusing:

Error Handling

If the RPC is called for a block we don't have yet, just return an error.

But below there seems to be a contradiction:

...

Funding streams for blocks we don't have yet

Use funding_stream_values(), and remove the miner fee field, because we don't know that yet.

I think we should do whatever zcashd does here.

@oxarbitrage I agree, which is why I added these open questions:

Is this RPC called before the block at height is created or validated by the node? What is the expected response? Is this RPC called for founders reward heights?

Can you find out what zcashd and s-nomp do here?

teor2345 commented 2 years ago

Thefounders field in this RPC is blocked on this question:

If it is, we'll need to do a lot more work for this ticket (add founders reward code, test it, and call it from the RPC).

oxarbitrage commented 2 years ago

I dont think this issue should be blocked. The blocked tag discourage people to keep researching. The investigation/question is part of the ticket and we want people to keep working on it, blocked means "get out" until something else is done and should be avoided.

oxarbitrage commented 2 years ago

I also don't think is a good idea to replace the proposed design with something else without some discussion. In my opinion my design was simpler than the one we have now. Instead of editing the OP unilaterally with a new design please add your comments as part of the discussion and then look for consensus to edit the ticket. This is only valid when more than one person is working on the design, should be ok to do whatever you want if you are the only one.

daira commented 1 year ago

I don't see why that open question can't be resolved as just "no, we don't need this RPC to work in zebrad for heights when the Founders' Reward was paid". So yes implementation of the "founders" field is blocked on that question (which is all that @teor2345 said), but it has an easy resolution.

oxarbitrage commented 1 year ago

Thank you @daira , i understand what you say but is a matter of internal zebra team tagging. I think the question is part of the ticket. When we see the blocked tag, developers ignore the ticket, which i don't think is what we should do here.

oxarbitrage commented 1 year ago

The label we have now (S-needs-investigation) is enough to describe that we have an open question. "Blocked" label is stronger and in general, a bad thing to add unilaterally if there is more than one person working in the design. It is just my perspective from a point of view of the receiver of that label (people outside the zebra team probably don't care about our labels).

oxarbitrage commented 1 year ago

I don't see why that open question can't be resolved as just "no, we don't need this RPC to work in zebrad for heights when the Founders' Reward was paid". So yes implementation of the "founders" field is blocked on that question (which is all that @teor2345 said), but it has an easy resolution.

Even more, we can leave the field empty initially so we can do the work in other fields we know we have to add. Then get back to it if we figure out it is actually needed for some use case.

teor2345 commented 1 year ago

I also don't think is a good idea to replace the proposed design with something else without some discussion. In my opinion my design was simpler than the one we have now. Instead of editing the OP unilaterally with a new design please add your comments as part of the discussion

You're right, I didn't realise you were still working on this, I'll revert my changes to the ticket description.

teor2345 commented 1 year ago

The ticket description has these issues:

Here are my suggested changes to the ticket:

Possible design

We need the fixed miner reward and funding stream values for future heights.

Fields:

We can get the funding stream recipient addresses and amounts fromfunding_stream_values(): https://github.com/ZcashFoundation/zebra/blob/dc9da24c8b6cee5c05315f0d77bee52613f75fd3/zebra-consensus/src/block/subsidy/funding_streams.rs#L21-L28

We'll need to move the subsidy module to zebra-node-services, so we can call it from the RPCs.

We can get the fixed miner reward from the subsidy module as well.

Error Handling

If the RPC is called for a founders reward block, just return an error.

Out of scope

Fields:

The RPC isn't called on existing blocks, so this field will always be zero.

teor2345 commented 1 year ago

@mpguerra I'm not sure if this RPC is in scope for our mining pool work, because it isn't required to mine blocks.

Instead, the coinbase transaction is returned by getblocktemplate.

We might want to stop working on this ticket until we're sure it is in scope. If we do decide to work on it, we might want to split the founders reward field into a separate ticket.

teor2345 commented 1 year ago

Something else I just discovered in the getblocktemplate RPC docs:

To obtain information about founder's reward or funding stream amounts, use 'getblocksubsidy HEIGHT' passing in the height returned by this API.

https://zcash.github.io/rpc/getblocktemplate.html

Since getblocktemplate heights are always new blocks, we can't get the getblocksubsidy amounts from mined blocks. Instead, we need to use the existing miner reward and funding stream functions to get them. (The founders reward will always be zero for new blocks.)

The miner field description is incorrect in the ticket, it is the mining reward only, not the transaction fees.

mpguerra commented 1 year ago

Hey team! Please add your planning poker estimate with Zenhub @arya2 @conradoplg @oxarbitrage @teor2345 @upbqdn

teor2345 commented 1 year ago

It's difficult to estimate this ticket when there are two very different alternative designs in it.

mpguerra commented 1 year ago

I'm going to remove this one from the GBT epic since we don't yet know of any usecases for it.

mpguerra commented 1 year ago

I don't think we'll be doing this in the end

teor2345 commented 1 year ago

s-nomp uses getblocksubsidy for Zcash via node-stratum-pool: https://github.com/s-nomp/node-stratum-pool/blob/aa3deddc8caa3ba5dfcff92c7db0f5360e812c10/lib/blockTemplate.js#L51-L63 https://github.com/s-nomp/node-stratum-pool/blob/d86ae73f8ff968d9355bb61aac05e0ebef36ccb5/lib/pool.js#L602-L608

But we don't know if any mining pools are using s-nomp yet.

teor2345 commented 1 year ago

I spoke with Pili and we are going to implement this RPC because some mining pools use it.