Skip to content

Conversation

@andrewtoth
Copy link
Contributor

@andrewtoth andrewtoth commented Dec 2, 2019

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.

There is some relevant conversation here.

@instagibbs
Copy link
Member

concept ACK, we want to test censorship type scenarios for Liquid, but don't have a good way of doing that in our own test harness aside from manually constructing blocks ourselves. Previously we relied on premature witness data being non-standard to "censor" the transactions themselves but that is no longer a non-standard case due to buried deployment.

Thoughts:

  1. Probably a bunch of code can be de-duplicated with some refactoring
  2. This should probably only be available for test networks, and hidden?

@andrewtoth andrewtoth force-pushed the generatecustomblock branch 4 times, most recently from bc0fd7f to 54d67c5 Compare December 2, 2019 20:50
@DrahtBot
Copy link
Contributor

DrahtBot commented Dec 2, 2019

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

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

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.

@andrewtoth
Copy link
Contributor Author

andrewtoth commented Dec 3, 2019

Probably a bunch of code can be de-duplicated with some refactoring

@instagibbs Most of the duplicated code to create the block is in BlockAssembler, and it's very tightly coupled with getting all txs from the mempool. I'm not sure it would be worth messing around with that code for this.

This should probably only be available for test networks, and hidden?

I don't believe this would have any effect if trying to generate on a chain with high difficulty, so not sure if there's any risk with letting it be used anywhere. The same could be said about generatetoaddress.

@instagibbs
Copy link
Member

I did a bit of refactoring that makes this re-use the existing mining code: https://github.com/instagibbs/bitcoin/tree/generatecustomblock

Fewer added lines, and less redundancy imo.

Feel free to take this approach instead.

Copy link
Contributor

@promag promag left a comment

Choose a reason for hiding this comment

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

Concept ACK.

{"transactions", RPCArg::Type::ARR, RPCArg::Optional::NO, "An array of hex strings which are either txids or raw transactions.\n"
"Txids must reference transactions currently in the mempool.",
{
{"rawtx/txid", RPCArg::Type::STR_HEX, RPCArg::Optional::OMITTED, ""},
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess it's fine supporting both here.

CMutableTransaction mtx;
if (ParseHashStr(str, hash)) {

LOCK(mempool.cs);
Copy link
Contributor

Choose a reason for hiding this comment

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

You can replace this code with CTxMemPool::get.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@andrewtoth andrewtoth closed this Dec 7, 2019
@andrewtoth andrewtoth deleted the generatecustomblock branch December 7, 2019 16:00
@andrewtoth andrewtoth changed the title rpc: Add generatecustomblock to mine a custom set of transactions rpc: Add generateblock to mine a custom set of transactions Dec 7, 2019
@andrewtoth
Copy link
Contributor Author

Closed by mistake. Reopened new PR #17693

maflcko pushed a commit to maflcko/bitcoin-core that referenced this pull request Apr 10, 2020
…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
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Apr 13, 2020
…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
PhotoshiNakamoto added a commit to PhotonicBitcoin/pBTC-core that referenced this pull request Dec 11, 2021
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.
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Dec 16, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants