rpc: generateblock to allow multiple outputs

polespinasa commented at 5:34 pm on May 10, 2025: contributor

Generatetomany

First approach was to create a generatetomany rpc call. That approach can be seen here https://github.com/polespinasa/bitcoin/pull/3

Generateblock

Modifies generateblock to allow a user to set multiple outputs in the coinbase transaction. If an empty set is provided, the block will be empty. If no set of transactions is provided, the block will mine the mempool. If a set of transactions is provided only that set of txs will be mined.

0$ ./bitcoin-cli -rpcport=18443 generateblock '["bcrt1qal6p633hvwz2yp5mav0qy7u2az8gkn2xywnj6v", "bcrt1qvr3qgyhw6y0e0zj97v0j5yc40xtpea4wqj0g43"]' []
1{
2  "hash": "74deaa8b6f677321a76c6508d57c47896da440db411663a89879c091ca08cbc4"
3}
4$ ./bitcoin-cli -rpcport=18443 generateblock '["bcrt1qal6p633hvwz2yp5mav0qy7u2az8gkn2xywnj6v", "bcrt1qvr3qgyhw6y0e0zj97v0j5yc40xtpea4wqj0g43"]'
5{
6  "hash": "55171d2db37a50cddb507d186da324e20227384cf0a201c38abb0bad65bf5d98"
7}

Motivation

https://x.com/SuperTestnet/status/1921220086645342550

Citating @supertestnet

When mining a block on regtest, the command “generatetoaddress” is available if you want to send the entire coinbase to 1 address. Let’s add generatetomany in case you want to split up the coinbase among multiple addresses, similar to how the “sendtoaddress” and “sendmany” commands both exist and let you send money to (1) one address or (2) multiple addresses. The generatetomany command would be particularly useful for simulating protocols that send money to multiple recipients directly from a coinbase, as several pools do now, like ocean and braidpool.

Notes

Needs release note
Changes on generateblock are not backwards compatible as now it takes an array of outputs instead a single output. This should not be critical as it is a test RPC.

DrahtBot commented at 5:34 pm on May 10, 2025: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/32468.

Reviews

See the guideline for information on the review process. A summary of reviews will appear here.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#32587 ([WIP] test: Fix reorg patterns in mempool tests to use proper fork-based approach by i-am-yuvi)
#32189 (refactor: Txid type safety (parent PR) by marcofleon)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

DrahtBot added the label RPC/REST/ZMQ on May 10, 2025

polespinasa force-pushed on May 10, 2025

polespinasa commented at 5:42 pm on May 10, 2025: contributor

I would like to discuss how to implement reward distribution. Do we ask the user for the exact number of bitcoins for each address or do we opt to use percentages of the reward?

IMHO it is better to use percentages, it makes things easier for the user.

DrahtBot added the label CI failed on May 10, 2025

DrahtBot commented at 5:48 pm on May 10, 2025: contributor

🚧 At least one of the CI tasks failed. Task tidy: https://github.com/bitcoin/bitcoin/runs/41993075774 LLM reason (✨ experimental): The CI failure is due to a build error related to missing members in node::BlockAssembler::Options.

Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:

Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.
An intermittent issue.

Leave a comment here, if you need help tracking down a confusing failure.

polespinasa force-pushed on May 10, 2025

andrewtoth commented at 5:56 pm on May 10, 2025: contributor

The generateblock command takes either an address or descriptor as its first argument. Perhaps it would be easier to extend that RPC to also take an array of address or descriptor and value pairs? That way we won’t need a new RPC.

polespinasa commented at 5:59 pm on May 10, 2025: contributor

Perhaps it would be easier to extend that RPC to also take an array of address or descriptor and value pairs?

That would be easy as the code is already done on the new RPC. But this change would not be backwards compatible, idk if we want to keep it that way.

There are some softwares like Polar that would be affected by this.

polespinasa force-pushed on May 10, 2025

andrewtoth commented at 6:17 pm on May 10, 2025: contributor

But this change would not be backwards compatible

Sure it would be. Just parse an array first and if that fails parse a string.

DrahtBot removed the label CI failed on May 10, 2025

polespinasa commented at 9:48 pm on May 10, 2025: contributor

Just parse an array first and if that fails parse a string.

Oh right, that’s an option. I will let it as it is for now to see other contributors’ opinions.

May take your approach later.

supertestnet commented at 11:38 pm on May 10, 2025: none

Can a PSBT be constructed with only outputs, no inputs? If so, then maybe generateblock could accept a psbt or a string as an argument. If it is a psbt, it must contain outputs but no inputs.

But also, due to fees being variable, it may be useful to have some kind of “remainder” option so that you can say, for example, “give 1 btc apiece to the first 3 addresses, and give the remainder to the last address”

polespinasa commented at 7:51 am on May 11, 2025: contributor

Can a PSBT be constructed with only outputs, no inputs?

I’m not sure using a PSBT is the best approach. If we check the normal workflow for a PSBT we see it is intended to:

Create the PSBT with inputs and outputs but no metadata
Updated with the data needed to spend the UTXOs in the inputs
Signed
Finalized

IMO using PSBTs here does not make sense, we don’t need any of that, we don’t need to share with other users or software to sign or add information to inputs. It’s easier to just provide a JSON with the addresses and amounts (it’s the only info we need), also using a JSON it’s easier to implement with less changes.

But also, due to fees being variable, it may be useful to have some kind of “remainder” option so that you can say, for example, “give 1 btc apiece to the first 3 addresses, and give the remainder to the last address”

Do you have any idea on how that could look like? Maybe something like this; where amount_fixed takes a number and share_reminder is a boolean: generatetomany 1 [{addr1, amount_fixed, share_remainder}, ...]

As per your example would be:

0generatetomany 1 [{bcrt1qal6p633hvwz2yp5mav0qy7u2az8gkn2xywnj6v, 1, 0}, {bcrt1q3me8uxjzw2egq76hjw6zcgzmlhlzzztfwqzjfv, 1, 0}, {bcrt1q57yu04dn4l0zxv6ul4prm7favr630d3lmwvjh0, 1, 0}, {bcrt1qal6p633hvwz2yp5mav0qy7u2az8gkn2xywnj6v, 0, 1}]

supertestnet commented at 7:57 am on May 11, 2025: none

Do you have any idea on how that could look like?

Here is how the syntax looks for the sendmany command which I am using as inspiration:

bitcoin-cli sendmany "" "{\"bc1q09vm5lfy0j5reeulh4x5752q25uqqvz34hufdl\":0.01,\"bc1q02ad21edsxd23d32dfgqqsz4vv4nmtfzuklhy3\":0.02}"

So maybe we could do something similar:

bitcoin-cli generatetomany "{\"bcrt1qal6p633hvwz2yp5mav0qy7u2az8gkn2xywnj6v\":1,\"bcrt1q3me8uxjzw2egq76hjw6zcgzmlhlzzztfwqzjfv\":1,\"bcrt1q57yu04dn4l0zxv6ul4prm7favr630d3lmwvjh0\":1,\"bcrt1qal6p633hvwz2yp5mav0qy7u2az8gkn2xywnj6v\":\"remainder\"}" <– note the remainder in the last btc address

polespinasa commented at 8:08 am on May 11, 2025: contributor

note the remainder in the last btc address

what if you want to split the reminder between multiple addresses?

lollerfirst commented at 11:09 am on May 11, 2025: none

note the remainder in the last btc address

what if you want to split the reminder between multiple addresses?

bitcoin-cli generatetomany 5 "{\"bcrt1qal6p633hvwz2yp5mav0qy7u2az8gkn2xywnj6v\":1,\"bcrt1q3me8uxjzw2egq76hjw6zcgzmlhlzzztfwqzjfv\":1,\"bcrt1q57yu04dn4l0zxv6ul4prm7favr630d3lmwvjh0\":1,\"bcrt1qal6p633hvwz2yp5mav0qy7u2az8gkn2xywnj6v\":\"remainder\", \"bcrt1qz07rr2j8r699c9r55rt44q9vm3faxgprlv62xx\":\"remainder\", \"bcrt1q9vur8vqhfndr8r76eveuu2wr46f2rs8fgv2kvh\":\"remainder\"}"

All fields with “remainder” to have the remainder split equally amongst them?

in src/node/miner.cpp:166 in fb28aea67d outdated

164-    coinbaseTx.vout[0].nValue = nFees + GetBlockSubsidy(nHeight, chainparams.GetConsensus());
165+
166+    const auto numOutputs = m_options.coinbase_outputs_scripts.size();
167+    coinbaseTx.vout.resize(numOutputs);
168+    if (!numOutputs) {
169+        coinbaseTx.vout[0].scriptPubKey = m_options.coinbase_outputs_scripts[0];

maflcko commented at 12:36 pm on May 11, 2025:

this is now dead code, according to https://corecheck.dev/bitcoin/bitcoin/pulls/32468

Generally, I am not sure about modifying real code to accommodate test-only code. It would be better to not modify src/node at all and just put the test-only code in the test-only code paths.

(Unless the code may be useful outside of tests, but then you’ll have to explain how)

polespinasa commented at 5:27 pm on May 11, 2025:

this is now dead code, according to https://corecheck.dev/bitcoin/bitcoin/pulls/32468

Will delete it, was just added as all the else statement code is not needed for the simeple generatetoaddress.

Generally, I am not sure about modifying real code to accommodate test-only code. It would be better to not modify src/node at all and just put the test-only code in the test-only code paths.

I’m not understanding this part, what do you mean by test-only code? I don’t see a way to add this functionality without modifying this src/node/… files. Care to explain more?

maflcko commented at 5:21 am on May 12, 2025:

test-only code is code that is only called by tests (unit, or functional tests, or on regtest, testnet, signet, …), but never on mainnet.

static UniValue generateBlocks is a test-only function, so a similar approach could be used here.

polespinasa commented at 7:01 am on May 12, 2025:

So you suggest duplicating CreateNewBlock with a test-only version? I don’t see how to implement the changes outside that function…

maflcko commented at 7:06 am on May 12, 2025:

It is possible to call CNB from the test-only function. Something like:

0def CNB_test()
1 b = CNB()
2 b.vtx[0].output_script = b'aa'
3 ... etc

polespinasa commented at 9:27 am on May 12, 2025:

Thanks for the review! Please see 5a5ff2a20cdff56b5756fa0b6c9cbb9184f17818 Tried to follow your approach, actually it makes things easier :)

Will squash commits after ack the new approach better than the first one.

lollerfirst commented at 3:34 pm on May 11, 2025: none

@polespinasa @supertestnet Please check out https://github.com/polespinasa/bitcoin/pull/1 with the discussed changes and let me know what you think.

Also maybe we would want to follow @andrewtoth advice and not create a new rpc command in the first place.

polespinasa commented at 5:30 pm on May 11, 2025: contributor

All fields with “remainder” to have the remainder split equally amongst them?

I don’t see duplicating entries as a good option. Could lead to errors. I think is better to have two camps for each address entry, one for the amount (can be 0) and one for reminder (can be done with 0 or 1 or by setting “remainder” or an empty string)

polespinasa commented at 5:31 pm on May 11, 2025: contributor

@polespinasa @supertestnet Please check out polespinasa#1 with the discussed changes and let me know what you think.

left some comments on your proposal PR

Also maybe we would want to follow @andrewtoth advice and not create a new rpc command in the first place.

Still not sure about this, would like to know more opinions, for other things like send we have multiple RPCs. Anyway, it’s an easy change, so it’s something we can do at the end if we want.

lollerfirst commented at 5:51 pm on May 11, 2025: none

duplicating entries

How is it duplicating entries? In the example there is a different address for each entry.

polespinasa commented at 5:54 pm on May 11, 2025: contributor

How is it duplicating entries?

First address is duplicated. Anyway it is duplicated in case you want to assign an amount to an address + remainder.

polespinasa force-pushed on May 12, 2025

DrahtBot added the label CI failed on May 12, 2025

polespinasa force-pushed on May 12, 2025

in src/rpc/mining.cpp:178 in 5a5ff2a20c outdated

175+        CAmount rewardParted = totalReward / numOutputs;
176+        CAmount remainder = totalReward % numOutputs;
177+
178+        CMutableTransaction mutable_coinbase(*block.vtx.at(0));
179+
180+        int witness_index = GetWitnessCommitmentIndex(block);

polespinasa commented at 9:26 am on May 12, 2025:

This is necessary to avoid RegenerateCommitments to return -1 so breaking the code. Idk if there’s a better approach to it.

polespinasa renamed this:
~~rpc:generatetomany~~
rpc: generatetomany
on May 12, 2025

DrahtBot removed the label CI failed on May 12, 2025

mzumsande commented at 2:45 pm on May 12, 2025: contributor

Could the motivation be added here? Doesn’t seem ok having to resort to twitter (where people without an account can’t even see most of the thread) to know the reason for a PR.

supertestnet commented at 4:24 pm on May 12, 2025: none

Here is my motivation:

When mining a block on regtest, the command “generatetoaddress” is available if you want to send the entire coinbase to 1 address. Let’s add generatetomany in case you want to split up the coinbase among multiple addresses, similar to how the “sendtoaddress” and “sendmany” commands both exist and let you send money to (1) one address or (2) multiple addresses. The generatetomany command would be particularly useful for simulating protocols that send money to multiple recipients directly from a coinbase, as several pools do now, like ocean and braidpool.

andrewtoth commented at 4:39 pm on May 12, 2025: contributor

Still not sure about this, would like to know more opinions, for other things like send we have multiple RPCs

Since I don’t seem to have convinced you, here are some more arguments for why this feature belongs in generateblock instead of a new RPC:

New RPCs require adding more code to test, and once added must go through a deprecation cycle to be removed (if they can be removed at all without disrupting too many users).
Consumers of the RPCs must write additional code for the new RPC, instead of simply modifying the argument type.
generateblock is much more flexible than generatetoaddress (or generatetodescriptor), since it takes the txs to include as the second argument. For advanced test cases, it is likely that users would want this functionality to test multiple output coinbases with blocks that mine non-standard txs or otherwise don’t mine the entire mempool. They would not have this capability with generatetomany as implemented here, but would have that for free with generateblock.
generateblock currently doesn’t collect transaction fees (https://github.com/bitcoin/bitcoin/issues/31684), so adding this functionality to it would fix this issue. Users could just call generateblock with an array containing a single address/descriptor:value pair that has the reward plus fees.
generateblock is capable of mining invalid blocks, since the txs to mine in the second argument can be any set of txs. We can similarly punt the calculation of the coinbase reward to the caller, so they must correctly calculate that all the values of the outputs add up to no more than the correct reward + fees. This removes the bikeshedding of whether to use remainder or split values up evenly.

polespinasa commented at 6:55 pm on May 12, 2025: contributor

generateblock is much more flexible than generatetoaddress (or generatetodescriptor), since it takes the txs to include as the second argument. For advanced test cases, it is likely that users would want this functionality to test multiple output coinbases with blocks that mine non-standard txs or otherwise don’t mine the entire mempool. They would not have this capability with generatetomany as implemented here, but would have that for free with generateblock.

This is a fair point, the only bad thing is that (if I’m not wrong) generateblock is thought to be used always with a given set of txs and that may not always be the use case. Maybe would be worth to implement it on both RPC?

For the first two points I understand them, but I don’t think it’s a problem. In case we consider those two reasons enough to not add a new PRC, this could be adapted to work inside generatetoaddress or generatetodescriptor adding the new arguments (payout amounts) and making them optional to keep the original behaviour if wanted. Making it backward compatible.

generateblock is capable of mining invalid blocks, since the txs to mine in the second argument can be any set of txs. We can similarly punt the calculation of the coinbase reward to the caller, so they must correctly calculate that all the values of the outputs add up to no more than the correct reward + fees. This removes the bikeshedding of whether to use remainder or split values up evenly.

This is also a good point but also focused on only this use case. Would say the same as before, you may want to mine txs from the mempool and not care about the exact amount of sats to distribute. Maybe implement this on both RPC is the solution to it.

What do you think?

andrewtoth commented at 7:34 pm on May 12, 2025: contributor

Consider if we already had the functionality for this in generateblock - what would be the motivation to add a new RPC generatetomany? Note that sendtoaddress and sendmany have both been superseded by send, but the older RPCs can’t easily be removed. sendall is an interesting case, which we might want to mimic instead of adding a remainder field. But, then that won’t fix #31684.

andrewtoth commented at 5:22 pm on May 13, 2025: contributor

generateblock is thought to be used always with a given set of txs and that may not always be the use case. Maybe would be worth to implement it on both RPC?

you may want to mine txs from the mempool and not care about the exact amount of sats to distribute. Maybe implement this on both RPC is the solution to it.

What do you think?

I think you are making the case to add a feature to generateblock to mine the mempool and collect rewards the same as generateto commands. Perhaps we can make the second argument of generateblock optional, and if there is no set of txs provided then mine the mempool.

I don’t think we should be duplicating the effort implementing this on more than one RPC. Then every new mining test feature needs to be duplicated (or triplicated for generatetodescriptor/generatetomany). We should deprecate generateto RPCs and just use generateblock for everything going forward, similar to send vs sendtoaddress/sendmany.

polespinasa commented at 7:27 am on May 14, 2025: contributor

Perhaps we can make the second argument of generateblock optional, and if there is no set of txs provided then mine the mempool

That could be a smart approach. But I would always take the mempool or also put an optional argument to mine the mempool so it is not one or the other.

I don’t think we should be duplicating the effort implementing this on more than one RPC. Then every new mining test feature needs to be duplicated (or triplicated for generatetodescriptor/generatetomany). We should deprecate generateto RPCs and just use generateblock for everything going forward, similar to send vs sendtoaddress/sendmany.

I might agree, will think how to build it around generateblock. Deprecation for the other RPCs can be done in another PR as I think will be a blocking reason for merging, don’t want it to influence this one.

polespinasa force-pushed on May 20, 2025

DrahtBot added the label CI failed on May 20, 2025

polespinasa force-pushed on May 20, 2025

polespinasa commented at 9:58 am on May 20, 2025: contributor

Perhaps we can make the second argument of generateblock optional, and if there is no set of txs provided then mine the mempool.

This is implemented here 8b5dd8a5f135ce1aaf80d8c28943f503f4ee19d4 With a small difference, if no set of txs is provided then we mine the mempool, if an empty set is provided we mine an empty block (original behaviour of the RPC).

Note: for the moment fee collector is not implemented if a set of tx is set manually

in src/rpc/mining.cpp:309 in 8b5dd8a5f1 outdated

386@@ -306,10 +387,16 @@ static RPCHelpMan generateblock()
387     return RPCHelpMan{"generateblock",
388         "Mine a set of ordered transactions to a specified address or descriptor and return the block hash.",
389         {
390-            {"output", RPCArg::Type::STR, RPCArg::Optional::NO, "The address or descriptor to send the newly generated bitcoin to."},
391-            {"transactions", RPCArg::Type::ARR, RPCArg::Optional::NO, "An array of hex strings which are either txids or raw transactions.\n"
392+            {"outputs", RPCArg::Type::ARR, RPCArg::Optional::NO, "The addresses or descriptor to split, in equal parts, the coinbase reward among.",

maflcko commented at 11:31 am on May 20, 2025:

not sure about forcing an array, when all tests mostly just want to provide a single dummy value.

it would be good to make this optional and then, require one of output or outputs to be present, or maybe even fallback to OP_TRUE if none are given? edit: Or maybe just fallback to OP_RETURN to avoid interfering with tests that assume utxo state?

polespinasa commented at 2:46 pm on May 20, 2025:

not sure about forcing an array, when all tests mostly just want to provide a single dummy value.

I don’t think this should be a problem. Same happens with sendall. Most tests just provide a single value in a list. And it actually doesn’t affect many tests.

I think it keeps the code simpler and avoids the confusion that could appear if we use multiple possible arguments.

Also, even if this is not a backward compatible change it’s not something critical as it is the test code.

But I like the idea of making it optional with a fallback to OP_RETURN if there are no values provided.

polespinasa commented at 11:33 am on May 26, 2025:

I’ve set “output” as optional but still I have to provide at least an empty list. I’m unable to see how to make it fully optional.

0$ ./bitcoin-cli -rpcport=18443 generateblock
1error code: -3
2error message:
3JSON value of type null is not of expected type array

0$ ./bitcoin-cli -rpcport=18443 generateblock []
1{
2  "hash": "25987bbfbce2be6c61393ed95340729ff2cc25c2b26db422b6e43c8a8f4558a0"
3}

maflcko commented at 12:08 pm on May 26, 2025:

get_array is a check function to ensure the type is array. (The type null is not the type array, so the check fails)

The fix would be to not call get_array when the type is null.

polespinasa commented at 1:16 pm on May 26, 2025:

oh you’re right, solved :)

DrahtBot removed the label CI failed on May 20, 2025

naiyoma commented at 10:36 am on May 24, 2025: contributor

I’m a bit confused about the current approach. Are you planning to add generatetomany and also extend generateblock to accommodate an array of addresses?

I suggest that you put one approach in your fork and indicate this clearly in your description—separation of concerns makes it easier to follow and review.

For example: Approach 1 Approach 2 → Link to the PR in your fork

polespinasa commented at 10:40 am on May 24, 2025: contributor

I’m a bit confused about the current approach. Are you planning to add generatetomany and also extend generateblock to accommodate an array of addresses?

generatetomany will not be added. I have to delete that code.

maflcko commented at 10:42 am on May 24, 2025: member

generatetomany will not be added. I have to delete that code.

Generally, it is best to just push the final version of the code, so that it is ready for further review. Prior versions of the code can trivially be retrieved via the commit hash (or a named alias), if needed.

in src/rpc/mining.cpp:354 in 571e37a74f outdated

350+             {
351+                 {RPCResult::Type::STR_HEX, "", "blockhash"},
352+             }},
353+        RPCExamples{
354+            "\nGenerate 11 blocks to two different addresses:\n"
355+            + HelpExampleCli("generatetomany", "\"11 '[\\\"bcrt1qal6p633hvwz2yp5mav0qy7u2az8gkn2xywnj6v\\\", \\\"bcrt1qvr3qgyhw6y0e0zj97v0j5yc40xtpea4wqj0g43\\\"]'\"")

naiyoma commented at 10:43 am on May 24, 2025:

11 shouldn’t be str

0--- a/src/rpc/mining.cpp
1+++ b/src/rpc/mining.cpp
2@@ -351,7 +351,7 @@ static RPCHelpMan generatetomany()
3             }},
4        RPCExamples{
5            "\nGenerate 11 blocks to two different addresses:\n"
6-            + HelpExampleCli("generatetomany", "\"11 '[\\\"bcrt1qal6p633hvwz2yp5mav0qy7u2az8gkn2xywnj6v\\\", \\\"bcrt1qvr3qgyhw6y0e0zj97v0j5yc40xtpea4wqj0g43\\\"]'\"")
7+            + HelpExampleCli("generatetomany", "11 '[\"bcrt1qal6p633hvwz2yp5mav0qy7u2az8gkn2xywnj6v\", \"bcrt1qvr3qgyhw6y0e0zj97v0j5yc40xtpea4wqj0g43\"]'")

polespinasa commented at 10:16 am on May 25, 2025:

Thx! Will correct in case we revert to generatetomany approach.

polespinasa force-pushed on May 25, 2025

polespinasa commented at 10:15 am on May 25, 2025: contributor

Squashed all commits and removed the first approach (generatetomany) which can still be checked here https://github.com/polespinasa/bitcoin/pull/3

polespinasa renamed this:
~~rpc: generatetomany~~
rpc: generateblock to allow multiple outputs
on May 25, 2025

in test/functional/rpc_txoutproof.py:72 in 67172f6b1b outdated

68@@ -69,7 +69,7 @@ def run_test(self):
69         # We can't get the proof if we specify a non-existent block
70         assert_raises_rpc_error(-5, "Block not found", self.nodes[0].gettxoutproof, [txid_spent], "0000000000000000000000000000000000000000000000000000000000000000")
71         # We can't get the proof if we only have the header of the specified block
72-        block = self.generateblock(self.nodes[0], output="raw(55)", transactions=[], submit=False)
73+        block = self.generateblock(self.nodes[0], outputs=["raw(55)"], transactions=[], submit=False)

maflcko commented at 3:25 pm on May 25, 2025:

all those raw( can probably just be omitted and use the OP_RETURN fallback

polespinasa commented at 11:31 am on May 26, 2025:

done!

polespinasa force-pushed on May 26, 2025

polespinasa commented at 11:32 am on May 26, 2025: contributor

OP_RETURN fallback if no address or descriptor set is provided.
A few tests added to test new functionalities

polespinasa force-pushed on May 26, 2025

in src/rpc/mining.cpp:310 in a4c0eecc8f outdated

305@@ -306,10 +306,17 @@ static RPCHelpMan generateblock()
306     return RPCHelpMan{"generateblock",
307         "Mine a set of ordered transactions to a specified address or descriptor and return the block hash.",
308         {
309-            {"output", RPCArg::Type::STR, RPCArg::Optional::NO, "The address or descriptor to send the newly generated bitcoin to."},
310-            {"transactions", RPCArg::Type::ARR, RPCArg::Optional::NO, "An array of hex strings which are either txids or raw transactions.\n"
311+            {"outputs", RPCArg::Type::ARR, RPCArg::Optional::OMITTED, "The addresses or descriptor to split, in equal parts, the coinbase reward among.\n"
312+                "If no outputs are provided the coinbase transaction will burn the coins into a OP_RETURN output.",

maflcko commented at 12:53 pm on May 27, 2025:

“into a OP_RETURN output.” -> “into an OP_RETURN output.” [article before vowel sound “O”]

I am also thinking about splitting up the “make this arg optional”. Seems like a small and easy preparatory pull request?

naiyoma commented at 1:00 pm on May 27, 2025: contributor

A summary of my understanding of this PR so far, regarding the generateblock changes: :

Outputs and transactions are now optional.
generateblock can now take an array of addresses and distribute the reward equally among them.
generateblock with an empty array - in this case fallback to OP_TRUE
I think there’s an intention to add a remainder — that way, it’s possible to specify part of the reward to specific addresses and then split the remainder among the other addresses

Generate 2 addresses

0HASH=$(./build/bin/bitcoin-cli generateblock '["bcrt1qkqjkxltw5s89kkpxg0v9560nuh8c3alu04f6d6", "bcrt1qke6n4hcu4pl2fmv9ueklnn7fz6knum2n0qcnvc"]' | jq -r .hash)
1
2# Verify that the reward is being split equally
3./build/bin/bitcoin-cli getrawtransaction $(./build/bin/bitcoin-cli getblock $HASH | jq -r .tx[0]) true | jq '.vout[] | select(.value > 0) | {address: .scriptPubKey.address, btc: .value}'

Test withan empty array

0HASH=$(./build/bin/bitcoin-cli generateblock '[]' | jq -r .hash)
1TXID=$(./build/bin/bitcoin-cli getblock $HASH | jq -r .tx[0])
2./build/bin/bitcoin-cli getrawtransaction $TXID true | jq '.vout'

still works with a single address and a transaction

0TXID=$(./build/bin/bitcoin-cli sendtoaddress $ADDRESS1 0.2)
1HASH=$(./build/bin/bitcoin-cli generateblock '["bcrt1qmvzwzg0hcym343w2u6gakqzp9s0kw2u964ga68"]' "[\"$TXID\"]" | jq -r .hash)
2./build/bin/bitcoin-cli getblock $HASH | jq '.tx'

in src/rpc/mining.cpp:345 in a4c0eecc8f outdated

341     CScript coinbase_output_script;
342     std::string error;
343+    std::vector<CScript> coinbase_outputs_scripts;
344+    if (address_or_descriptor.empty()) {
345+        coinbase_outputs_scripts.push_back(CScript() << OP_RETURN);
346+        //throw JSONRPCError(RPC_INVALID_PARAMETER, "Error: at least one address or descriptor must be provided.");

naiyoma commented at 1:01 pm on May 27, 2025:

Delete this instead of commenting it out.

in src/rpc/mining.cpp:411 in a4c0eecc8f outdated

408+        if(!mine_mempool) {
409+            CHECK_NONFATAL(block.vtx.size() == 1);
410+            block.vtx.insert(block.vtx.end(), txs.begin(), txs.end());
411+        }
412+
413+        const auto numOutputs = coinbase_outputs_scripts.size();

naiyoma commented at 1:04 pm on May 27, 2025:

nit: Snake case is preferred for variables. https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md#coding-style-c

achow101 commented at 10:59 pm on May 28, 2025: member

OP_RETURN fallback if no address or descriptor set is provided.

Why? I think it would be simpler to just require that at least one output address/descriptor is specified.

in src/rpc/mining.cpp:339 in a4c0eecc8f outdated

335@@ -329,42 +336,56 @@ static RPCHelpMan generateblock()
336         },
337         [&](const RPCHelpMan& self, const JSONRPCRequest& request) -> UniValue
338 {
339-    const auto address_or_descriptor = request.params[0].get_str();
340+    const auto address_or_descriptor = request.params[0].isNull() ? UniValue(UniValue::VARR) : request.params[0].get_array();

achow101 commented at 11:01 pm on May 28, 2025:

Although this RPC is only used by tests, I don’t think we should be breaking backwards compatibility here. It’s equally easy to determine whether this parameter is a string, so we can accept a string here for backwards compatibility.

The reason to keep compatibility is because this RPC has a high likelihood of being used in scripts and automated tests. Changing the type of the parameter will break all of those.

polespinasa commented at 9:37 am on May 29, 2025:

Is that possible without adding a new argument for single strings?

achow101 commented at 4:01 pm on May 29, 2025:

Yes, see verbose|verbosity in getblock for example.

in src/rpc/mining.cpp:312 in a4c0eecc8f outdated

305@@ -306,10 +306,17 @@ static RPCHelpMan generateblock()
306     return RPCHelpMan{"generateblock",
307         "Mine a set of ordered transactions to a specified address or descriptor and return the block hash.",
308         {
309-            {"output", RPCArg::Type::STR, RPCArg::Optional::NO, "The address or descriptor to send the newly generated bitcoin to."},
310-            {"transactions", RPCArg::Type::ARR, RPCArg::Optional::NO, "An array of hex strings which are either txids or raw transactions.\n"
311+            {"outputs", RPCArg::Type::ARR, RPCArg::Optional::OMITTED, "The addresses or descriptor to split, in equal parts, the coinbase reward among.\n"
312+                "If no outputs are provided the coinbase transaction will burn the coins into a OP_RETURN output.",
313+                {
314+                    {"address", RPCArg::Type::STR, RPCArg::Optional::OMITTED, "A valid address"},

achow101 commented at 11:01 pm on May 28, 2025:

Descriptors are accepted too.

DrahtBot added the label CI failed on May 30, 2025

rpc: generateblock with multiple outputs 57964a7267

allow single address or descriptor as a string bd2c8b255a

polespinasa force-pushed on May 30, 2025

rpc: generateblock to allow multiple outputs #32468

Generatetomany

Generateblock

Motivation

Notes

Code Coverage & Benchmarks

Reviews

Conflicts