rpc: Add generateblock to mine a custom set of transactions #17693

andrewtoth · 2019-12-07T17:32:38Z

The existing block generation rpcs for regtest, generatetoaddress and generatetodescriptor, mine everything in the mempool up to the block weight limit. This makes it difficult to test a system for several scenarios where a different set of transactions are mined. For example:

Testing the common scenario where a transaction is replaced in the mempool but the replaced transaction is mined instead.
Testing for a double-spent transaction where a transaction that conflicts with the mempool is mined.
Testing for non-standard transactions that are mined.
Testing the scenario where several blocks are mined without a specific transaction in the mempool being included in a block.

This PR introduces a new rpc, generateblock, that takes an array of raw transactions and txids and mines only those and the coinbase. Any txids must be in the mempool, but the raw txs can be anything conforming to consensus rules. The coinbase can be specified as either an address or descriptor.

This reopens #17653 since it was closed by mistake.

Thanks to instagibbs for code suggestions that I used here.

DrahtBot · 2019-12-07T23:13:36Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

rpc: remove deprecated CRPCCommand constructor #18531 (rpc: Assert that RPCArg names are equal to CRPCCommand ones by MarcoFalke)

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.

luke-jr · 2020-01-03T19:06:31Z

~~Concept NACK,~~ just make the block and submit it in the test suite...

andrewtoth · 2020-01-05T16:14:34Z

@luke-jr What if there is no "test suite"? Usually when testing a system that talks to bitcoind with regtest I use cli to send from bitcoind wallet and generatetoaddress to mine blocks. With this setup it is very difficult to simulate double spend without having a second bitcoind and doing an extensive workaround. This seems to have a decent amount of interest, since this PR had a thumbs up and the original PR #17653 had two concept ACKs. Also, #16523 was created for this same purpose.

luke-jr · 2020-01-05T16:37:13Z

Fair enough. Concept NACK retracted.

instagibbs · 2020-02-20T15:23:49Z

concept ACK, we need it to test our systems as well

src/rpc/mining.cpp

src/miner.cpp

sipa · 2020-03-17T00:10:20Z

Concept ACK on having an RPC that can do this, but I'm not convinced about the approach (see comment elsewhere).

instagibbs · 2020-03-17T01:06:00Z

I'll take another look this week. Maybe I need to see the approaches side by side.

…

On Mon, Mar 16, 2020, 8:40 PM andrewtoth ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In src/miner.cpp <#17693 (comment)>: > nFees = 0; } Optional<int64_t> BlockAssembler::m_last_block_num_txs{nullopt}; Optional<int64_t> BlockAssembler::m_last_block_weight{nullopt}; -std::unique_ptr<CBlockTemplate> BlockAssembler::CreateNewBlock(const CScript& scriptPubKeyIn) +std::unique_ptr<CBlockTemplate> BlockAssembler::CreateNewBlock(const CScript& scriptPubKeyIn, const std::vector<CTransactionRef>* preselected_txs) I did initially b0745f8 <b0745f8> but @instagibbs <https://github.com/instagibbs> suggested I take this approach instead #17653 (comment) <#17653 (comment)>. Now I'm not sure which is better, duplicating a lot of code from BlockAssembler or modifying it to support adding preselected txs. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#17693 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABMAFU44MHTZUALQHO6WUSTRH3BJVANCNFSM4JXP3YZQ> .

sipa · 2020-03-17T01:09:03Z

Ok, I see. But the first iteration of the code was duplicating everything, including the grinding. I'm suggesting not duplicating the grinding, but by just constructing a BlockTemplate directly, instead of overloading BlockAssembler to do that.

andrewtoth · 2020-03-17T04:34:50Z

@sipa updated with your suggestion.

maflcko

ACK

src/rpc/mining.cpp

instagibbs · 2020-03-17T14:37:35Z

src/rpc/mining.cpp

comment for followup only: Allow a bypass of validity check for building block tests via RPC

src/rpc/mining.cpp

maflcko · 2020-03-27T16:16:30Z

re-ACK 7524b64 📁

Show signature and timestamp

Signature:

Timestamp of file with hash df2de0a97ae347b1985267b6969efcbe2e76c6b0a484832ef46013bf7f784ade -

andrewtoth · 2020-03-27T17:24:52Z

Does this need release notes?

maflcko · 2020-03-27T18:36:46Z

A one-line release note in the "tests" section should be sufficient. This can be done after merge, since it is not clear if this makes it into 0.20 or 0.21

It only affects test, so it might make it into 0.20, but I wanted to wait for other reviewers first.

Adjusted the upstream bitcoin/bitcoin#17693 for auxpow.

…ransactions 7524b64 Add tests for generateblock (Andrew Toth) dcc8332 Add generateblock rpc (Andrew Toth) Pull request description: The existing block generation rpcs for regtest, `generatetoaddress` and `generatetodescriptor`, mine everything in the mempool up to the block weight limit. This makes it difficult to test a system for several scenarios where a different set of transactions are mined. For example: - Testing the common scenario where a transaction is replaced in the mempool but the replaced transaction is mined instead. - Testing for a double-spent transaction where a transaction that conflicts with the mempool is mined. - Testing for non-standard transactions that are mined. - Testing the scenario where several blocks are mined without a specific transaction in the mempool being included in a block. This PR introduces a new rpc, `generateblock`, that takes an array of raw transactions and txids and mines only those and the coinbase. Any txids must be in the mempool, but the raw txs can be anything conforming to consensus rules. The coinbase can be specified as either an address or descriptor. This reopens bitcoin#17653 since it was closed by mistake. Thanks to instagibbs for code suggestions that I used here. ACKs for top commit: MarcoFalke: re-ACK 7524b64 📁 Tree-SHA512: 857106007465b5b9b8a84b6d07c17cbf8378a33a72d32ff79abea1d5ab4babb4d53a11ddbb14595aa1fac9dfa1391e3a11403d742f69951beea2f683e8a01cd4

Summary: ``` The existing block generation rpcs for regtest, generatetoaddress and generatetodescriptor, mine everything in the mempool up to the block weight limit. This makes it difficult to test a system for several scenarios where a different set of transactions are mined. For example: Testing the common scenario where a transaction is replaced in the mempool but the replaced transaction is mined instead. Testing for a double-spent transaction where a transaction that conflicts with the mempool is mined. Testing for non-standard transactions that are mined. Testing the scenario where several blocks are mined without a specific transaction in the mempool being included in a block. This PR introduces a new rpc, generateblock, that takes an array of raw transactions and txids and mines only those and the coinbase. Any txids must be in the mempool, but the raw txs can be anything conforming to consensus rules. The coinbase can be specified as either an address or descriptor. ``` Backport of core [[bitcoin/bitcoin#17693 | PR17693]]. The code has been adapted to remove the Segwit parts. For now the RPC interface is kept identical to core, but it would be interesting to not enforce CTOR in the call and sort the txs in the RPC code instead to make it easier to mix raw txs and txids. Test Plan: ninja check-functional Reviewers: #bitcoin_abc, deadalnix Reviewed By: #bitcoin_abc, deadalnix Differential Revision: https://reviews.bitcoinabc.org/D8553

jnewbery · 2021-03-27T10:55:54Z

src/rpc/mining.cpp

-        if (pblock->nNonce == std::numeric_limits<uint32_t>::max()) {
-            continue;


This change means that the extra_nonce logic is now unused. Previously if the nNonce field rolled, we'd go around the loop again and call IncrementExtraNonce(). Now we'll just return true.

I've proposed removing that logic entirely in #21533.

Never mind, I misunderstood the new interface for GenerateBlock():

returns false: generation of block failed

returns true with valid block_hash out-param: generation of block succeeded

returns true with invalid (nullified) block_hash out-param: generation of block didn't succeed, maximum nNonce reached

(thanks to @theStack for pointing this out: #21533 (review))

Bitcoin Core PR: bitcoin/bitcoin#17693 Pull request description: The existing block generation rpcs for regtest, `generatetoaddress` and `generatetodescriptor`, mine everything in the mempool up to the block weight limit. This makes it difficult to test a system for several scenarios where a different set of transactions are mined. For example: - Testing the common scenario where a transaction is replaced in the mempool but the replaced transaction is mined instead. - Testing for a double-spent transaction where a transaction that conflicts with the mempool is mined. - Testing for non-standard transactions that are mined. - Testing the scenario where several blocks are mined without a specific transaction in the mempool being included in a block. This PR introduces a new rpc, `generateblock`, that takes an array of raw transactions and txids and mines only those and the coinbase. Any txids must be in the mempool, but the raw txs can be anything conforming to consensus rules. The coinbase can be specified as either an address or descriptor. This reopens bitcoin/bitcoin#17653 since it was closed by mistake. Thanks to instagibbs for code suggestions that I used here.

…ctions

fanquake added the RPC/REST/ZMQ label Dec 7, 2019

andrewtoth mentioned this pull request Dec 7, 2019

rpc: Add generateblock to mine a custom set of transactions #17653

Closed

andrewtoth force-pushed the generateblock branch 3 times, most recently from ec5ce9f to 7d93244 Compare December 10, 2019 22:49

andrewtoth force-pushed the generateblock branch from 7d93244 to 5ad549b Compare December 27, 2019 23:26

DrahtBot added the Needs rebase label Jan 2, 2020

andrewtoth force-pushed the generateblock branch from 5ad549b to a9bff39 Compare January 2, 2020 23:24

DrahtBot removed the Needs rebase label Jan 3, 2020

andrewtoth force-pushed the generateblock branch from a9bff39 to 643d351 Compare January 5, 2020 17:01

This was referenced Feb 11, 2020

Return BlockValidationState from ProcessNewBlock if CheckBlock/AcceptBlock fails #17479

Closed

BIP-325: Signet support #16411

Closed

Log RPC parameters (arguments) if -debug=rpcparams #16365

Closed

fanquake mentioned this pull request Feb 25, 2020

Rpc removemempoolentry #15873

Closed

andrewtoth mentioned this pull request Feb 28, 2020

Add removemempoolentry RPC to evict transactions from the mempool #16523

Closed

maflcko reviewed Mar 16, 2020

View reviewed changes

src/rpc/mining.cpp Outdated Show resolved Hide resolved

sipa reviewed Mar 17, 2020

View reviewed changes

src/miner.cpp Outdated Show resolved Hide resolved

andrewtoth force-pushed the generateblock branch from 643d351 to 718796a Compare March 17, 2020 04:11

maflcko approved these changes Mar 17, 2020

View reviewed changes

src/rpc/mining.cpp Outdated Show resolved Hide resolved

src/rpc/mining.cpp Outdated Show resolved Hide resolved

src/rpc/mining.cpp Outdated Show resolved Hide resolved

instagibbs reviewed Mar 17, 2020

View reviewed changes

andrewtoth force-pushed the generateblock branch from 718796a to 8becb08 Compare March 22, 2020 21:50

Add tests for generateblock

7524b64

andrewtoth force-pushed the generateblock branch from 4292064 to 7524b64 Compare March 27, 2020 16:09

DrahtBot mentioned this pull request Apr 5, 2020

rpc: remove deprecated CRPCCommand constructor #18531

Merged

maflcko merged commit 51e2ce4 into bitcoin:master Apr 10, 2020

domob1812 added a commit to domob1812/namecoin-core that referenced this pull request Apr 13, 2020

Merge branch 'bitcoin' into auxpow

6431393

Adjusted the upstream bitcoin/bitcoin#17693 for auxpow.

maflcko mentioned this pull request May 10, 2020

rpc: Add submit option to generateblock #18933

Merged

jnewbery mentioned this pull request Mar 27, 2021

mining: Remove unused extranonce logic #21533

Closed

jnewbery reviewed Mar 27, 2021

View reviewed changes

kwvg added a commit to kwvg/dash that referenced this pull request Apr 5, 2022

merge bitcoin#17693: Add generateblock to mine a custom set of transa…

a2aa72d

…ctions

kwvg added a commit to kwvg/dash that referenced this pull request Apr 19, 2022

merge bitcoin#17693: Add generateblock to mine a custom set of transa…

eea2d83

…ctions

bitcoin locked as resolved and limited conversation to collaborators Aug 18, 2022

andrewtoth deleted the generateblock branch August 17, 2023 20:40

		if (pblock->nNonce == std::numeric_limits<uint32_t>::max()) {
		continue;

rpc: Add generateblock to mine a custom set of transactions #17693

rpc: Add generateblock to mine a custom set of transactions #17693

Uh oh!

Conversation

andrewtoth commented Dec 7, 2019 • edited by fanquake Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Dec 7, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

luke-jr commented Jan 3, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

andrewtoth commented Jan 5, 2020

Uh oh!

luke-jr commented Jan 5, 2020

Uh oh!

instagibbs commented Feb 20, 2020

Uh oh!

Uh oh!

Uh oh!

sipa commented Mar 17, 2020

Uh oh!

instagibbs commented Mar 17, 2020 via email

Uh oh!

sipa commented Mar 17, 2020

Uh oh!

andrewtoth commented Mar 17, 2020

Uh oh!

maflcko left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

instagibbs Mar 17, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

maflcko commented Mar 27, 2020

Uh oh!

andrewtoth commented Mar 27, 2020

Uh oh!

maflcko commented Mar 27, 2020

Uh oh!

jnewbery Mar 27, 2021

Choose a reason for hiding this comment

Uh oh!

jnewbery Apr 14, 2021

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

andrewtoth commented Dec 7, 2019 •

edited by fanquake

Loading

DrahtBot commented Dec 7, 2019 •

edited

Loading

luke-jr commented Jan 3, 2020 •

edited

Loading