ZcashFoundation / zebra

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

change(rpc): Ensure mining pools will accept responses to `getblocksubsidy` #8702

Closed upbqdn closed 2 months ago

upbqdn commented 3 months ago

The response of getblocksubsidy contains an array, named fundingstreams, where each object contains a pair named address. If the community chooses the lockbox as a funding stream, there will be no real address to use in that field. We can either:

  1. skip the serialization of that pair,
  2. set its value to the empty string, or
  3. use a dummy address.

In any case, we should make sure mining pools will accept the responses.

daira commented 3 months ago

I propose the following:

Rationale:

nuttycom commented 3 months ago

@daira I named the array lockboxstreams in https://github.com/zcash/zcash/pull/6912/files#diff-ccc24453c13307f815879738d3bf00eec351417537fbf10dde1468180cacd2f1. The suggestion of the top-level total is a good one.

nuttycom commented 3 months ago

I don't think that total_required_outputs is a good name though, because the lockbox value explicitly doesn't correspond to a transaction output, which could be confusing. It's more like total_non_miner_block_subsidy or something.

oxarbitrage commented 3 months ago

I like the proposal. we need to decide if we should keep returning the fundingstreams array after Nu6 and if we do, with what inside (empty or some sort of dummy addresses with 0 as value)

oxarbitrage commented 3 months ago

For reference, here are a few samples we have of getblocksubsidy outputs we did in the past for zebrad at different heights (which should be the same as zcashd):

The work in progress proposal for the format after Nu6 is the following (feel free to edit):

{
  "fundingstreams": [],
  "lockboxstreams": [
    {
      "recipient": "Funding LockBox 1",
      "specification": "https://zips.z.cash/draft-nuttycom-funding-allocation",
      "value": 0.21875,
      "valueZat": 21875000,
    }
  ],
  "miner": 2.5,
  "founders": 0.0,
  "lockboxtotal":0.0,
  "fundingstreamstotal": 0.0,
  "totalblocksubsidy": 0.0,
}
upbqdn commented 3 months ago

One thing I didn't mention in the PR description is that the consumers of the RPC might have hardcoded expectations regarding the response content.

I noticed this with https://github.com/nighthawk-apps/zcash-explorer, which was expecting both result and error in RPC responses because zcashd returns such responses. Zebra returns either result or error, which corresponds to the JSON-RPC spec (https://github.com/ZcashFoundation/zebra/issues/8672#issuecomment-2229018550), and the block explorer couldn't parse Zebra's responses. It occurred to me that a similar issue might happen here with mining pools if we changed the response format.

nuttycom commented 3 months ago

I think that instead of total_required_outputs, we should have a totalblocksubsidy (no underscores, for consistency with the rest of the API) such that totalblocksubsidy = miner + founders + Σ fundingstreams.value + Σ lockboxstreams.value

oxarbitrage commented 3 months ago

I think that instead of total_required_outputs, we should have a totalblocksubsidy (no underscores, for consistency with the rest of the API) such that totalblocksubsidy = miner + founders + Σ fundingstreams.value + Σ lockboxstreams.value

Ok, i changed that in the proposed output in https://github.com/ZcashFoundation/zebra/issues/8702#issuecomment-2248773668

nuttycom commented 3 months ago

A couple more additions, for completeness:

+            "  \"fundingstreamstotal\" : x.xxx, (numeric) The total value of direct funding streams in " + CURRENCY_UNIT + ".\n"
+            "  \"lockboxtotal\" : x.xxx,        (numeric) The total value sent to development funding lockboxes in " + CURRENCY_UNIT + ".\n"
+            "  \"totalblocksubsidy\" : x.xxx,   (numeric) The total value of the block subsidy in " + CURRENCY_UNIT + ".\n"
daira commented 3 months ago

"totalblocksubsidy" makes sense to me in place of "total_required_outputs" (which I agree is badly named), because you can always compute totalblocksubsidy - miner if you want the total amount directed to other places by consensus.