Skip to content

rpc: Add submit option to generateblock #18933

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
fanquake merged 2 commits into from
Mar 23, 2023
Merged

rpc: Add submit option to generateblock #18933

fanquake merged 2 commits into from
Mar 23, 2023

Conversation

@maflcko
Copy link
Member

@maflcko maflcko commented May 10, 2020

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
Copy link
Member Author

maflcko commented May 10, 2020

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

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

@maflcko maflcko added the Tests label May 10, 2020
@andrewtoth
Copy link
Contributor

andrewtoth commented May 10, 2020

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.

@maflcko
Copy link
Member Author

maflcko commented May 10, 2020

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
jonatack reviewed May 10, 2020
View reviewed changes
Copy link
Member

@jonatack jonatack left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does removing the generatetodescriptor RPC require a deprecation cycle?

@maflcko
Copy link
Member Author

maflcko commented May 10, 2020

Does removing the generatetodescriptor RPC require a deprecation cycle?

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

@andrewtoth
Copy link
Contributor

andrewtoth commented May 10, 2020

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
Copy link
Member Author

maflcko commented May 10, 2020

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

@DrahtBot
Copy link
Contributor

DrahtBot commented May 10, 2020 •
edited
Loading

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.

This was referenced May 10, 2020
Merged
Merged
Closed
instagibbs
instagibbs reviewed May 11, 2020
View reviewed changes
Copy link
Member

@instagibbs instagibbs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK aside from the json question

@andrewtoth
Copy link
Contributor

andrewtoth commented May 12, 2020 •
edited
Loading

The comment this is based on (#17693 (comment)) 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?

@maflcko maflcko force-pushed the 2004-rpcBlock branch 2 times, most recently from fa0857c to faf2727 Compare December 15, 2021 16:06
@DrahtBot DrahtBot mentioned this pull request Jan 11, 2022
Closed
@DrahtBot DrahtBot mentioned this pull request Jan 25, 2022
Merged
@DrahtBot DrahtBot mentioned this pull request Mar 19, 2022
Merged
@DrahtBot DrahtBot mentioned this pull request Apr 25, 2022
Merged
@DrahtBot DrahtBot mentioned this pull request May 19, 2022
Merged
@DrahtBot DrahtBot mentioned this pull request Aug 4, 2022
Merged
@DrahtBot DrahtBot mentioned this pull request Aug 13, 2022
Closed
@achow101
Copy link
Member

achow101 commented Oct 12, 2022

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 achow101 closed this Oct 12, 2022
@maflcko maflcko reopened this Mar 10, 2023
@maflcko maflcko mentioned this pull request Mar 10, 2023
Closed
@maflcko
Copy link
Member Author

maflcko commented Mar 10, 2023

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?

sedited
sedited approved these changes Mar 21, 2023
View reviewed changes
Copy link
Contributor

@sedited sedited left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tACK fa18504

@instagibbs
Copy link
Member

instagibbs commented Mar 23, 2023

ACK fa18504

@DrahtBot DrahtBot removed the request for review from instagibbs March 23, 2023 12:21
@fanquake fanquake merged commit 8acfb1f into bitcoin:master Mar 23, 2023
@maflcko maflcko deleted the 2004-rpcBlock branch March 23, 2023 17:13
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Mar 23, 2023
@fanquake
Merge bitcoin#18933: rpc: Add submit option to generateblock
caceb36
@bitcoin bitcoin locked and limited conversation to collaborators Mar 22, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@sedited sedited sedited approved these changes

@pinheadmz pinheadmz Awaiting requested review from pinheadmz

+2 more reviewers

@jonatack jonatack jonatack left review comments

@instagibbs instagibbs instagibbs left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Tests

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

8 participants

@maflcko @andrewtoth @DrahtBot @achow101 @instagibbs @jonatack @sedited @fanquake