zebra icon indicating copy to clipboard operation
zebra copied to clipboard

Add support for `getblocksubsidy` RPC call

Open mpguerra opened this issue 2 years ago • 17 comments

Motivation

Mining pools use the getblocksubsidy RPC calls.

Requirements

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

Height argument

  • is optional.
  • is a number (current zebra functions that have height as argument are a string).

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?

Possible design

We can get everything for this call from the requested block coinbase transaction:

We can get the recipient address from the lock_script field of each output.

  • miner: Is the amount of any output of the coinbase that is not a founders reward or funding stream address.
  • founders: Is the amount of any output which is a founders rewards address. **
  • fundingstreams: Amount can be obtained by output with a funding stream address, other data for the array is hardcoded.

** We need to add founders rewards addresses to zebra (we removed them long time ago as we are not validating).

Testing

  • [ ] Snapshot the output of the RPC
  • [ ] Test calling the RPC from the mining pool software
  • [ ] Manually compare the zcashd and zebrad RPC output using https://github.com/ZcashFoundation/zebra/blob/main/zebra-utils/zcash-rpc-diff

mpguerra avatar Sep 29 '22 08:09 mpguerra

@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 avatar Oct 06 '22 00:10 teor2345

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

teor2345 avatar Oct 06 '22 00:10 teor2345

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

teor2345 avatar Oct 06 '22 00:10 teor2345

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 avatar Oct 06 '22 13:10 oxarbitrage

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 avatar Oct 06 '22 21:10 teor2345

Thefounders field in this RPC is blocked on this question:

  • Is this RPC called for founders reward heights?

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

teor2345 avatar Oct 06 '22 23:10 teor2345

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 avatar Oct 07 '22 15:10 oxarbitrage

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.

oxarbitrage avatar Oct 07 '22 15:10 oxarbitrage

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.

daira avatar Oct 09 '22 15:10 daira

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 avatar Oct 09 '22 15:10 oxarbitrage

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 avatar Oct 09 '22 15:10 oxarbitrage

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.

oxarbitrage avatar Oct 09 '22 16:10 oxarbitrage

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 avatar Oct 10 '22 05:10 teor2345

The ticket description has these issues:

  • the consensus rules are incorrect: the funding stream and founders reward amounts are required at least once in the coinbase transaction, it's not the total amount sent to the address
  • the funding stream amounts are hard-coded for every height, we already have a Rust function to calculate them
  • the RPC uses the fixed miner reward, not the variable mining fees
  • the RPC is called for block heights that haven't been created yet
  • we need to work out how to handle errors

Here are my suggested changes to the ticket:

Possible design

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

Fields:

  • fundingstreams: The required amount sent to each funding stream address, the specification is hard-coded
  • miner: the required amount of the miner reward
  • foundersreward: out of scope for now, the RPC is not called for existing blocks

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:

  • founders: The required amount sent to a founders rewards address

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

teor2345 avatar Oct 10 '22 05:10 teor2345

@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 avatar Oct 10 '22 05:10 teor2345

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.

teor2345 avatar Oct 10 '22 20:10 teor2345

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

mpguerra avatar Oct 14 '22 08:10 mpguerra

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

teor2345 avatar Oct 19 '22 04:10 teor2345

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

mpguerra avatar Nov 29 '22 13:11 mpguerra

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

mpguerra avatar Jan 24 '23 09:01 mpguerra

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 avatar Jan 24 '23 19:01 teor2345

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

teor2345 avatar Jan 24 '23 23:01 teor2345