rpc: Add submit option to generateblock #18933

maflcko commented at 2:18 PM on May 10, 2020: member

When submit is turned off, a block can be generated and returned as hex, to be used for further tests. For example, it can be submitted on a different node, on a different interface (like p2p), or just never submitted and be used for other testing purposes.

maflcko commented at 2:20 PM on May 10, 2020: member

Requested by @instagibbs, I believe, in comment #17693 (review)

This work is based on the generateblock RPC added by @andrewtoth in #17693

maflcko added the label Tests on May 10, 2020

andrewtoth commented at 4:35 PM on May 10, 2020: contributor

generatetodescriptor will be available for at least 2 releases. Is there any concern that other users will depend on it? I suppose workarounds for it are easy enough.

in doc/release-notes.md:107 in fa872b5f6e outdated

 103 | @@ -104,6 +104,9 @@ Low-level changes
 104 |  Tests
 105 |  -----
 106 |  
 107 | +- The `genereratetodescriptor` RPC has been removed and replaced by the

jonatack commented at 4:46 PM on May 10, 2020:

s/genereratetodescriptor/generatetodescriptor/

maflcko commented at 4:55 PM on May 10, 2020:

Thanks, fixed.

maflcko commented at 4:47 PM on May 10, 2020: member

If anyone was quick enough to add generatetodescriptor to their test scripts, they are surely quick enough to pull it out again the next time they update their node to a new major version.

jonatack commented at 4:48 PM on May 10, 2020: contributor

Does removing the generatetodescriptor RPC require a deprecation cycle?

maflcko commented at 4:53 PM on May 10, 2020: member

Does removing the generatetodescriptor RPC require a deprecation cycle?

I'd say no, because it is only used for testing. See also #18933 (comment)

maflcko force-pushed on May 10, 2020

andrewtoth commented at 5:50 PM on May 10, 2020: contributor

I haven't used generatetodescriptor myself, but I would imagine a use case would be to mine transactions in the mempool with it. With generateblock a user has to manually add all txids to get the same functionality. If we are replacing it, should we also add an option to generateblock to mine the mempool?

maflcko force-pushed on May 10, 2020

maflcko commented at 6:23 PM on May 10, 2020: member

Ok, that can be done as a follow-up. Dropped the last commit faefc219b63825b3393ccea683645ecbbac0e169 because it was too controversial.

DrahtBot commented at 9:36 PM on May 10, 2020: contributor

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

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	TheCharlatan, instagibbs

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

No conflicts as of last run.

DrahtBot cross-referenced this on May 10, 2020 from issue Make g_chainman internal to validation by maflcko

DrahtBot cross-referenced this on May 11, 2020 from issue rpc: remove deprecated CRPCCommand constructor by maflcko

DrahtBot cross-referenced this on May 11, 2020 from issue Return BlockValidationState from ProcessNewBlock if CheckBlock/AcceptBlock fails by jnewbery

in src/rpc/mining.cpp:1203 in fa58b4cd18 outdated

1199 | @@ -1187,7 +1200,7 @@ static const CRPCCommand commands[] =
1200 |  
1201 |      { "generating",         "generatetoaddress",      &generatetoaddress,      {"nblocks","address","maxtries"} },
1202 |      { "generating",         "generatetodescriptor",   &generatetodescriptor,   {"num_blocks","descriptor","maxtries"} },
1203 | -    { "generating",         "generateblock",          &generateblock,          {"output","transactions"} },
1204 | +    { "generating",         "generateblock",          &generateblock,          {"output","transactions","submit"} },

instagibbs commented at 2:20 PM on May 11, 2020:

Also should be added to "src/rpc/client.cpp" to be parsed as non-string on -cli, right?

maflcko commented at 2:24 PM on May 11, 2020:

Doh. Can't wait until RPCMan does that for me

#18531

instagibbs commented at 2:25 PM on May 11, 2020:

oh nice, will review

maflcko commented at 4:53 PM on December 15, 2021:

Thanks, fixed.

instagibbs commented at 2:21 PM on May 11, 2020: member

ACK aside from the json question

andrewtoth commented at 3:25 AM on May 12, 2020: contributor

The comment this is based on (https://github.com/bitcoin/bitcoin/pull/17693#discussion_r393727104) suggests to bypass validity check. However, https://github.com/bitcoin/bitcoin/pull/18933/files#diff-9651347c8e00bed3ddc7631de569406dL364 still does that. Do we want to skip that check as well?

DrahtBot added the label Needs rebase on May 23, 2020

maflcko force-pushed on Dec 15, 2021

DrahtBot removed the label Needs rebase on Dec 15, 2021

maflcko force-pushed on Dec 15, 2021

DrahtBot cross-referenced this on Jan 11, 2022 from issue doc: Explain in the generate* RPC docs that they are only for testing by sipa

DrahtBot cross-referenced this on Jan 25, 2022 from issue doc: Rework generate* doc by maflcko

DrahtBot added the label Needs rebase on Feb 21, 2022

maflcko force-pushed on Feb 22, 2022

DrahtBot removed the label Needs rebase on Feb 22, 2022

DrahtBot cross-referenced this on Mar 19, 2022 from issue deploymentstatus: move g_versionbitscache global to ChainstateManager by ajtowns

DrahtBot added the label Needs rebase on Mar 25, 2022

maflcko force-pushed on Mar 30, 2022

DrahtBot removed the label Needs rebase on Mar 30, 2022

maflcko force-pushed on Mar 31, 2022

DrahtBot cross-referenced this on Apr 1, 2022 from issue Remove buggy and confusing IncrementExtraNonce by maflcko

DrahtBot added the label Needs rebase on Apr 6, 2022

maflcko force-pushed on Apr 6, 2022

DrahtBot removed the label Needs rebase on Apr 6, 2022

DrahtBot cross-referenced this on Apr 25, 2022 from issue Remove not needed clang-format off comments by maflcko

DrahtBot added the label Needs rebase on May 13, 2022

maflcko force-pushed on May 13, 2022

DrahtBot removed the label Needs rebase on May 13, 2022

DrahtBot cross-referenced this on May 19, 2022 from issue refactor: Avoid passing params where not needed by maflcko

DrahtBot added the label Needs rebase on May 20, 2022

maflcko force-pushed on May 27, 2022

DrahtBot removed the label Needs rebase on May 27, 2022

DrahtBot cross-referenced this on Aug 4, 2022 from issue p2p: Implement anti-DoS headers sync by sdaftuar

DrahtBot cross-referenced this on Aug 13, 2022 from issue consensus: Remove mainnet checkpoints by sdaftuar

DrahtBot added the label Needs rebase on Aug 30, 2022

achow101 commented at 6:08 PM on October 12, 2022: member

Closing this as it has not had any activity in a while. If you are interested in continuing work on this, please leave a comment so that it can be reopened.

achow101 closed this on Oct 12, 2022

maflcko reopened this on Mar 10, 2023

maflcko force-pushed on Mar 10, 2023

refactor: Replace block_hash with block_out fab9a08e14

rpc: Add submit option to generateblock fa18504d57

maflcko force-pushed on Mar 10, 2023

maflcko cross-referenced this on Mar 10, 2023 from issue test: getblock on header throws by maflcko

maflcko commented at 10:02 AM on March 10, 2023: member

Do we want to skip that check as well?

Should be trivial to add in a follow-up with a one-line patch, if and when needed?

DrahtBot removed the label Needs rebase on Mar 10, 2023

TheCharlatan approved

TheCharlatan commented at 10:14 AM on March 21, 2023: contributor

tACK fa18504d5767a40dc9827fb081633219bf251001

fanquake requested review from instagibbs on Mar 23, 2023

fanquake requested review from pinheadmz on Mar 23, 2023

instagibbs commented at 12:21 PM on March 23, 2023: member

ACK fa18504d5767a40dc9827fb081633219bf251001

DrahtBot removed review request from instagibbs on Mar 23, 2023

fanquake merged this on Mar 23, 2023

fanquake closed this on Mar 23, 2023

maflcko deleted the branch on Mar 23, 2023

sidhujag referenced this in commit caceb360ad on Mar 23, 2023

bitcoin locked this on Mar 22, 2024