Drop script_pub_key arg from createNewBlock #31318

Sjors · 2024-11-18T15:31:37Z

Providing a script for the coinbase transaction is only done in test code and for (unoptimized) CPU solo mining.

Production miners use the getblocktemplate RPC which omits the coinbase transaction entirely from its block template, leaving it to external (pool) software to construct it.

This commit removes the script_pub_key argument from createNewBlock() in the Mining interface.

A coinbase script can still be passed via BlockCreateOptions instead. Tests are modified to do so.

DrahtBot · 2024-11-18T15:31:42Z

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/31318.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	ryanofsky, TheCharlatan, vasild

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#31283 (Add waitNext() to BlockTemplate interface by Sjors)
#31196 (Prune mining interface by Sjors)
#30391 (BlockAssembler: return selected packages virtual size and fee by ismaelsadeeq)
#30157 (Fee Estimation via Fee rate Forecasters by ismaelsadeeq)
#29415 (Broadcast own transactions only via short-lived Tor or I2P connections by vasild)

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.

Sjors · 2024-11-18T15:37:30Z

The PR was prompted because I noticed in #31283 I had to needlessly pass script_pub_key around.

DrahtBot · 2024-11-18T15:39:39Z

🚧 At least one of the CI tasks failed.
_{Debug: https://github.com/bitcoin/bitcoin/runs/33145905317}

Hints

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.

src/node/types.h

Sjors · 2024-11-18T16:11:39Z

Renamed to coinbase_output_script, added warning and discouragement, fixed fuzz binary build.

Sjors · 2024-11-19T10:43:02Z

Forgot to modify mining.capnp.

sedited · 2024-11-19T13:00:35Z

I'm not sure why this is really an improvement. Can you elaborate on why no external user would want to pass in a script pubkey directly, instead of manually manipulating the coinbase in the block template?

Sjors · 2024-11-19T13:56:39Z

Afaik the only way external users make blocks today is the getblocktemplate RPC, which drops the coinbase transaction from the template it returns.

Similarly in submitSolution, used by Stratum v2, we expect to receive the full coinbase transaction and replace whatever was in the CBlockTemplate.

Unless you're solo mining, the pool is going to add its own outputs, manipulate the extra nonce, add its own name, etc. Even with a solo pool (a weird concept) you would configure the payout address in that software, not in the node.

There is a way in Stratum v2 to specify a list of outputs that you want in the coinbase. In my current implementation we use this to include the witness commitment, but nothing else.

In the longer run we could expand the interface to specify more outputs.

The special case for that is a single payout address that takes all the money from the block. That could be used for solo mining with a single small ASIC that doesn't need to grind the extra nonce (up to about 100 TH/s with BIP320). But that's fairly niche use case. As soon as you use Stratum v1 / v2 software you typically pass a payout address to the the pool software, not to the node.

ryanofsky

Concept ACK . It seems like a nice improvement to not require specifying an option that won't typically be used. But I'm a little hazy on the details of this. I think better documentation could help. Left a suggestion and some questions below.

src/node/types.h

Sjors · 2024-11-19T19:49:53Z

Rebased to avoid spurious CI failure.

I'll take a look later if for any of the tests I can drop their custom coinbase output.

sedited · 2024-11-19T21:37:32Z

Re #31318 (comment)

Thank you for spelling this out @Sjors. I also like ryanofsky's idea of adding the OP_TRUE script as the default option instead of making it a std::optional.

Concept ACK

ryanofsky

Code review ACK 1dfc54d87c7b35cc93cfb81cc2a46c9e8eae7194. Left a style suggestion, and a suggestion to split up the first commit, if we want to do that. But these can be ignored and this all looks good as-is. Makes a lot of sense to not make a parameter which should only be used for testing a required argument.

ryanofsky · 2024-11-21T02:43:19Z

src/node/miner.cpp

In commit "Drop script_pub_key arg from createNewBlock" (42f880aeb080070a42229d0bccd517100a3fe7fb)

Would suggest using '*' instead of .value() since this code isn't intending to trigger the std::bad_optional_access exception, and to make it more obvious a dereference is happening.

I changed coinbase_output_script to no longer be std::optional.

src/node/miner.cpp

Sjors · 2024-11-21T09:30:37Z

Rebased after #31122.

I changed coinbase_output_script from std::optional to having a default of OP_TRUE.

I split the main commit into three parts:

Remove scriptPubKeyIn arg from CreateNewBlock, using a temporary overload to avoid test changes
Dropping the temp overload and change the tests
Change the interface

Providing a script for the coinbase transaction is only done in test code and for CPU solo mining. Production miners use the getblocktemplate RPC which omits the coinbase transaction entirely from its block template, leaving it to external (pool) software to construct it. A coinbase script can still be passed via BlockCreateOptions instead. A temporary overload is added so that the test can be modified in the next commit.

This removes the temporary overload added in the previous commit. Also drop unneeded custom coinbase output scripts.

Sjors · 2024-12-04T05:47:53Z

Rebased after #31112.

ryanofsky

Code review ACK 52fd151. No change since last review other than rebase

sedited

Re-ACK 52fd151

vasild

ACK 52fd151

Looks like a nice cleanup of the interface.

vasild · 2024-12-16T13:54:24Z

src/node/types.h

 //! @file node/types.h is a home for public enum and struct type definitions
-//! that are used by internally by node code, but also used externally by wallet
-//! or GUI code.
+//! that are used by internally by node code, but also used externally by wallet,


nit, was there before this PR, if you retouch, you can drop the excess "by":

Suggested change

//! that are used by internally by node code, but also used externally by wallet,

//! that are used internally by node code, but also used externally by wallet,

This was referenced Nov 18, 2024

Stratum v2 via IPC Mining Interface tracking issue #31098

Closed

Add waitNext() to BlockTemplate interface #31283

Merged

Sjors force-pushed the 2024/11/no-script_pub_key branch from 7879dbf to 523bde0 Compare November 18, 2024 15:39

DrahtBot added the CI failed label Nov 18, 2024

maflcko reviewed Nov 18, 2024

View reviewed changes

src/node/types.h Outdated Show resolved Hide resolved

Sjors force-pushed the 2024/11/no-script_pub_key branch from 523bde0 to bfdef23 Compare November 18, 2024 16:11

This was referenced Nov 19, 2024

Prune mining interface #31196

Merged

cluster mempool: Implement changeset interface for mempool #31122

Merged

BlockAssembler: return selected packages virtual size and fee #30391

Merged

Fee Estimation via Fee rate Forecasters #30157

Closed

Sjors force-pushed the 2024/11/no-script_pub_key branch from bfdef23 to ab2e06d Compare November 19, 2024 10:33

DrahtBot mentioned this pull request Nov 19, 2024

Broadcast own transactions only via short-lived Tor or I2P connections #29415

Merged

4 tasks

DrahtBot removed the CI failed label Nov 19, 2024

ryanofsky approved these changes Nov 19, 2024

View reviewed changes

src/node/types.h Outdated Show resolved Hide resolved

Sjors force-pushed the 2024/11/no-script_pub_key branch from ab2e06d to 42f880a Compare November 19, 2024 19:47

DrahtBot added the Needs rebase label Nov 20, 2024

ryanofsky approved these changes Nov 21, 2024

View reviewed changes

DrahtBot requested a review from sedited November 21, 2024 02:59

Sjors force-pushed the 2024/11/no-script_pub_key branch from 1dfc54d to d06f23b Compare November 21, 2024 09:30

DrahtBot removed the Needs rebase label Nov 21, 2024

DrahtBot added the Needs rebase label Dec 3, 2024

Sjors added 3 commits December 4, 2024 12:44

rpc: rename coinbase_script to coinbase_output_script

7ab733e

test: drop scriptPubKeyIn arg from CreateNewBlock

52fd151

This removes the temporary overload added in the previous commit. Also drop unneeded custom coinbase output scripts.

Sjors force-pushed the 2024/11/no-script_pub_key branch from 027ce3e to 52fd151 Compare December 4, 2024 05:47

DrahtBot removed the Needs rebase label Dec 4, 2024

ryanofsky reviewed Dec 9, 2024

View reviewed changes

DrahtBot requested a review from sedited December 9, 2024 21:33

sedited approved these changes Dec 9, 2024

View reviewed changes

Sjors mentioned this pull request Dec 16, 2024

Stratum v2 Template Provider common functionality Sjors/bitcoin#49

Closed

15 tasks

Sjors requested a review from maflcko December 16, 2024 07:19

vasild approved these changes Dec 16, 2024

View reviewed changes

ryanofsky merged commit cd3d9fa into bitcoin:master Dec 17, 2024
18 checks passed

	//! that are used by internally by node code, but also used externally by wallet,
	//! that are used internally by node code, but also used externally by wallet,

Drop script_pub_key arg from createNewBlock #31318

Drop script_pub_key arg from createNewBlock #31318

Uh oh!

Conversation

Sjors commented Nov 18, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Nov 18, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Coverage & Benchmarks

Reviews

Conflicts

Uh oh!

Sjors commented Nov 18, 2024

Uh oh!

DrahtBot commented Nov 18, 2024

Uh oh!

Uh oh!

Sjors commented Nov 18, 2024

Uh oh!

Sjors commented Nov 19, 2024

Uh oh!

sedited commented Nov 19, 2024

Uh oh!

Sjors commented Nov 19, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Sjors commented Nov 19, 2024

Uh oh!

sedited commented Nov 19, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

ryanofsky Nov 21, 2024

Choose a reason for hiding this comment

Uh oh!

Sjors Nov 21, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Sjors commented Nov 21, 2024

Uh oh!

Sjors commented Dec 4, 2024

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

sedited left a comment

Choose a reason for hiding this comment

Uh oh!

vasild left a comment

Choose a reason for hiding this comment

Uh oh!

vasild Dec 16, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Sjors commented Nov 18, 2024 •

edited

Loading

DrahtBot commented Nov 18, 2024 •

edited

Loading

Sjors commented Nov 19, 2024 •

edited

Loading

sedited commented Nov 19, 2024 •

edited

Loading