-
Notifications
You must be signed in to change notification settings - Fork 37.7k
refactor: add coinbase constraints to BlockAssembler::Options #30356
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.
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
Conversation
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code CoverageFor detailed information about the code coverage, see the test coverage report. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update. ConflictsReviewers, this pull request conflicts with the following ones:
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approach ACK
This adds flexibility for SV2, and having less magic numbers is better. I support adding a configuration parameter for this (would prevent others from needing to fork).
Left a couple small nits for now, but plan to test/exercise in more detail.
src/interfaces/mining.h
Outdated
@@ -42,9 +43,13 @@ class Mining | |||
* | |||
* @param[in] script_pub_key the coinbase output | |||
* @param[in] use_mempool set false to omit mempool transactions | |||
* @param[in] coinbase_output_max_additional_size maximum additional serialized bytes which the pool will add in coinbase transaction outputs |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: Is it comprehensive using the term "pool" here, or would it be better to keep things generic (and perhaps mention a pool as an example)? This is reserving space in the template for the coinbase and the entity requesting the template could be a pool or a solo miner (albeit less commonly), right? Maybe something like maximum additional serialized bytes the block requestor can add in coinbase transaction outputs (for example, to enable pool payouts)
.
nit: weight and bytes seem to be used interchangeably here and in the function definition. It might prevent confusion to stick to one nomenclature.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree something like "block requestor" is more generic than "pool", but also more confusing.
weight and bytes seem to be used interchangeably here and in the function definition
These bytes all go in the output which doesn't have a witness discount, so each byte counts as 4 weight units. I think we should prefer the term "bytes", but make sure it's used correctly since we use weight units for internal accounting.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Which has a few more hairy details:
The Template Provider MUST NOT provide NewMiningJob messages which would represent consensus-invalid blocks once this additional size — along with a maximally-sized (100 byte) coinbase field — is added. Further, the Template Provider MUST consider the maximum additional bytes required in the output count variable-length integer in the coinbase transaction when complying with the size limits.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I ended up renaming coinbase_output_max_additional_size
to coinbase_max_additional_weight
to account for these issues, see below.
So I guess it should ideally only have the kernel as a dependency? It is not quite clear to me yet how this is going to be achieved. The current mining interface requires knowledge of a EDIT: To be clear, since the constants are given a new header, there is no new hard library dependency introduced no matter which directory you choose to put them into. It just feels weird to put something very specific to the inner workings of our mining code into a directory called |
@TheCharlatan happy to put the constants somewhere else... see Sjors#47 for an attempt at introducing A similar problem might exist for |
4c57f27
to
3d45921
Compare
I renamed
The spec should probably be clarified a bit, cc @TheBlueMatt, @Fi3. In any case, tracking coinbase reserved space in term of weight units here should be enough to support Stratum v2 later, and even allow for using the coinbase witness. |
3d45921
to
d89637e
Compare
Rebased after #30324. |
Is there a reason why
has to be inside the block assembler class? Why not construct them externally and pass a reference of the options to the |
It doesn't need to be inside
That sounds fine. |
Weak approach NACK. This is just limiting the max block weight, which is already a configurable parameter. Having two params for the same thing seems like a poor way to do it. I'm not sure it avoids the risk of bugs producing invalid blocks, either - it just moves the bug risk from one place to another equally probable. Instead, maybe just perform the block weight cap on initialization rather than block assembly? |
This had me confused initially, and I tried to combine them, but there's really two different things:
With stratum v1 the distinction isn't very important. We just subtract (2) from (1). We could indeed do that at initialization. But with Stratum v2 (2) is a per-client setting communicated to us (the Template Provider) via the CoinbaseOutputDataSize message. We can set it at connection time, but not during node initialization.
The fact that Stratum v2 makes this configurable introduces that risk. But removing that ability from the protocol would probably just cause miners to roll their own extension and do it anyway. At 500 sats/byte and $50K per coin, squeezing in an extra 1000 vbyte gets you $250 per block. Pennies in front of steamrollers perhaps, but we've seen miners do this with the current code.
Happy to take patches. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Concept ACK d89637e
Giving the option to assign the values of coinbase_max_additional_weight
and coinbase_output_max_additional_sigops
needed by the Stratum V2 protocol is essential for its integration. I understand the risks involved by having two parameters for the same thing:
1. The max block weight demanded by the user via -blockmaxweight 2. Weight units reserved for the coinbase
but it seems unavoidable at a first glance due to their different invocation domains. During the node initialisation for the former and connection time for the later.
Left some small nit that only applies if the intended name for the variable was coinbase_max_additional_weight
instead of coinbase_max_addition_weight
src/interfaces/mining.h
Outdated
@@ -42,9 +43,13 @@ class Mining | |||
* | |||
* @param[in] script_pub_key the coinbase output | |||
* @param[in] use_mempool set false to omit mempool transactions | |||
* @param[in] coinbase_max_addition_weight maximum additional weight which the pool will add to the coinbase transaction |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I take it you wanted to rename it to coinbase_max_additional_weight
instead
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks.
d89637e
to
e67e466
Compare
Code review ACK e67e466, but I think the current implementation is too complicated and has too many layers of indirection. Would suggest applying the following patch and squashing the PR down to a single commit which would be just +41/-16 with the suggested changes: diff
--- a/src/Makefile.am
+++ b/src/Makefile.am
@@ -313,7 +313,6 @@ BITCOIN_CORE_H = \
util/hasher.h \
util/insert.h \
util/macros.h \
- util/mining.h \
util/moneystr.h \
util/overflow.h \
util/overloaded.h \
--- a/src/interfaces/mining.h
+++ b/src/interfaces/mining.h
@@ -5,10 +5,11 @@
#ifndef BITCOIN_INTERFACES_MINING_H
#define BITCOIN_INTERFACES_MINING_H
+#include <node/types.h>
+#include <uint256.h>
+
#include <memory>
#include <optional>
-#include <uint256.h>
-#include <util/mining.h>
namespace node {
struct CBlockTemplate;
@@ -42,14 +43,10 @@ public:
* Construct a new block template
*
* @param[in] script_pub_key the coinbase output
- * @param[in] use_mempool set false to omit mempool transactions
- * @param[in] coinbase_max_additional_weight maximum additional weight which the pool will add to the coinbase transaction
- * @param[in] coinbase_output_max_additional_sigops maximum additional sigops which the pool will add in coinbase transaction outputs
+ * @param[in] options options for creating the block
* @returns a block template
*/
- virtual std::unique_ptr<node::CBlockTemplate> createNewBlock(const CScript& script_pub_key, bool use_mempool = true,
- size_t coinbase_max_additional_weight = DEFAULT_COINBASE_MAX_ADDITIONAL_WEIGHT,
- size_t coinbase_output_max_additional_sigops = DEFAULT_COINBASE_OUTPUT_MAX_ADDITIONAL_SIGOPS) = 0;
+ virtual std::unique_ptr<node::CBlockTemplate> createNewBlock(const CScript& script_pub_key, const node::BlockCreateOptions& options={}) = 0;
/**
* Processes new block. A valid new block is automatically relayed to peers.
--- a/src/node/interfaces.cpp
+++ b/src/node/interfaces.cpp
@@ -883,25 +883,11 @@ public:
return TestBlockValidity(state, chainman().GetParams(), chainman().ActiveChainstate(), block, tip, /*fCheckPOW=*/false, check_merkle_root);
}
- std::unique_ptr<CBlockTemplate> createNewBlock(const CScript& script_pub_key, bool use_mempool,
- size_t coinbase_max_additional_weight,
- size_t coinbase_output_max_additional_sigops) override
+ std::unique_ptr<CBlockTemplate> createNewBlock(const CScript& script_pub_key, const BlockCreateOptions& options) override
{
- BlockAssembler::Options options;
- ApplyArgsManOptions(gArgs, options);
-
- Assume(coinbase_max_additional_weight <= DEFAULT_BLOCK_MAX_WEIGHT);
- options.coinbase_max_additional_weight = coinbase_max_additional_weight;
-
- Assume(options.coinbase_output_max_additional_sigops <= MAX_BLOCK_SIGOPS_COST);
- options.coinbase_output_max_additional_sigops = coinbase_output_max_additional_sigops;
-
- // The BlockAssembler constructor calls ClampOptions which clamps
- // nBlockMaxWeight between coinbase_output_max_additional_size and
- // DEFAULT_BLOCK_MAX_WEIGHT.
- // In other words, coinbase (reserved) outputs can safely exceed
- // -blockmaxweight, but the rest of the block template will be empty.
- return BlockAssembler{chainman().ActiveChainstate(), use_mempool ? context()->mempool.get() : nullptr, options}.CreateNewBlock(script_pub_key);
+ BlockAssembler::Options assemble_options{options};
+ ApplyArgsManOptions(*Assert(m_node.args), assemble_options);
+ return BlockAssembler{chainman().ActiveChainstate(), context()->mempool.get(), assemble_options}.CreateNewBlock(script_pub_key);
}
NodeContext* context() override { return &m_node; }
--- a/src/node/miner.cpp
+++ b/src/node/miner.cpp
@@ -59,14 +59,17 @@ void RegenerateCommitments(CBlock& block, ChainstateManager& chainman)
static BlockAssembler::Options ClampOptions(BlockAssembler::Options options)
{
+ Assume(options.coinbase_max_additional_weight <= DEFAULT_BLOCK_MAX_WEIGHT);
+ Assume(options.coinbase_output_max_additional_sigops <= MAX_BLOCK_SIGOPS_COST);
// Limit weight to between coinbase_max_additional_weight and DEFAULT_BLOCK_MAX_WEIGHT for sanity:
+ // Coinbase (reserved) outputs can safely exceed -blockmaxweight, but the rest of the block template will be empty.
options.nBlockMaxWeight = std::clamp<size_t>(options.nBlockMaxWeight, options.coinbase_max_additional_weight, DEFAULT_BLOCK_MAX_WEIGHT);
return options;
}
BlockAssembler::BlockAssembler(Chainstate& chainstate, const CTxMemPool* mempool, const Options& options)
: chainparams{chainstate.m_chainman.GetParams()},
- m_mempool{mempool},
+ m_mempool{options.use_mempool ? mempool : nullptr},
m_chainstate{chainstate},
m_options{ClampOptions(options)}
{
--- a/src/node/miner.h
+++ b/src/node/miner.h
@@ -6,6 +6,7 @@
#ifndef BITCOIN_NODE_MINER_H
#define BITCOIN_NODE_MINER_H
+#include <node/types.h>
#include <policy/policy.h>
#include <primitives/block.h>
#include <txmempool.h>
@@ -20,8 +21,6 @@
#include <boost/multi_index/tag.hpp>
#include <boost/multi_index_container.hpp>
-#include <util/mining.h>
-
class ArgsManager;
class CBlockIndex;
class CChainParams;
@@ -155,24 +154,13 @@ private:
Chainstate& m_chainstate;
public:
- struct Options {
+ struct Options : BlockCreateOptions {
// Configuration parameters for the block size
size_t nBlockMaxWeight{DEFAULT_BLOCK_MAX_WEIGHT};
CFeeRate blockMinFeeRate{DEFAULT_BLOCK_MIN_TX_FEE};
// Whether to call TestBlockValidity() at the end of CreateNewBlock().
bool test_block_validity{true};
bool print_modified_fee{DEFAULT_PRINT_MODIFIED_FEE};
- /**
- * The maximum additional weight which the pool will add to the coinbase
- * scriptSig, witness and outputs. This must include any additional
- * weight needed for larger CompactSize encoded lengths.
- */
- size_t coinbase_max_additional_weight{DEFAULT_COINBASE_MAX_ADDITIONAL_WEIGHT};
- /**
- * The maximum additional sigops which the pool will add in coinbase
- * transaction outputs.
- */
- size_t coinbase_output_max_additional_sigops{DEFAULT_COINBASE_OUTPUT_MAX_ADDITIONAL_SIGOPS};
};
explicit BlockAssembler(Chainstate& chainstate, const CTxMemPool* mempool, const Options& options);
--- a/src/node/types.h
+++ b/src/node/types.h
@@ -13,6 +13,8 @@
#ifndef BITCOIN_NODE_TYPES_H
#define BITCOIN_NODE_TYPES_H
+#include <cstddef>
+
namespace node {
enum class TransactionError {
OK, //!< No error
@@ -24,6 +26,24 @@ enum class TransactionError {
MAX_BURN_EXCEEDED,
INVALID_PACKAGE,
};
+
+struct BlockCreateOptions {
+ /**
+ * Set false to omit mempool transactions
+ */
+ bool use_mempool{true};
+ /**
+ * The maximum additional weight which the pool will add to the coinbase
+ * scriptSig, witness and outputs. This must include any additional
+ * weight needed for larger CompactSize encoded lengths.
+ */
+ size_t coinbase_max_additional_weight{4000};
+ /**
+ * The maximum additional sigops which the pool will add in coinbase
+ * transaction outputs.
+ */
+ size_t coinbase_output_max_additional_sigops{400};
+};
} // namespace node
#endif // BITCOIN_NODE_TYPES_H
--- a/src/rpc/mining.cpp
+++ b/src/rpc/mining.cpp
@@ -371,7 +371,7 @@ static RPCHelpMan generateblock()
ChainstateManager& chainman = EnsureChainman(node);
{
- std::unique_ptr<CBlockTemplate> blocktemplate{miner.createNewBlock(coinbase_script, /*use_mempool=*/false)};
+ std::unique_ptr<CBlockTemplate> blocktemplate{miner.createNewBlock(coinbase_script, {.use_mempool = false})};
if (!blocktemplate) {
throw JSONRPCError(RPC_INTERNAL_ERROR, "Couldn't create new block");
}
deleted file mode 100644
--- a/src/util/mining.h
+++ /dev/null
@@ -1,13 +0,0 @@
-// Copyright (c) 2024 The Bitcoin Core developers
-// Distributed under the MIT software license, see the accompanying
-// file COPYING or http://www.opensource.org/licenses/mit-license.php.
-
-#ifndef BITCOIN_UTIL_MINING_H
-#define BITCOIN_UTIL_MINING_H
-
-/** Default for coinbase_max_additional_weight */
-static constexpr unsigned int DEFAULT_COINBASE_MAX_ADDITIONAL_WEIGHT{4000};
-/** Default for coinbase_output_max_additional_sigops */
-static constexpr unsigned int DEFAULT_COINBASE_OUTPUT_MAX_ADDITIONAL_SIGOPS{400};
-
-#endif // BITCOIN_UTIL_MINING_H Suggested changes:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code review ACK c504b69
src/node/miner.cpp
Outdated
Assume(options.coinbase_max_additional_weight <= DEFAULT_BLOCK_MAX_WEIGHT); | ||
Assume(options.coinbase_output_max_additional_sigops <= MAX_BLOCK_SIGOPS_COST); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
re: #30356 (comment)
Just to clarify for other reviewers since the names of these macros are confusing: In bitcoin core, assert
and Assert
macros behave very differently from the assert
macro in standard c++. In standard c++ assert
statements are not executed at all in release builds, while in bitcoin core they are always executed and always cause the execution to abort if they fail. Other c++ libraries use the term check instead of assert for this behavior.
The Assume
macro is more unusual, because it doesn't abort execution in release builds, but does also doesn't get skipped in release builds, so will always evaluate arguments passed to it. It can make sense to use Assume instead of assert when an unexpected condition indicates some kind of bug, but the bug is easy to recover from, so it is worth crashing debug builds but not release builds. Normally it make sense to check the Assume
return value and do something like log a warning or fix invalid values, not just ignore the unexpected condition completely.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code review ACK c504b69
The PR description needs to be updated to match the PR.
Left a single question
* Set false to omit mempool transactions | ||
*/ | ||
bool use_mempool{true}; | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is their a reason we are not adding coinbase_script
into the BlockCreateOptions
struct?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No strong reason, but the coinbase script is different for every block, unlike the other stuff we pass to BlockCreateOptions
. It seems worthy of a positional argument.
Add interface changes from bitcoin#30409 bitcoin#30356 bitcoin#30440 bitcoin#30443 This commit updates the Cap'n Proto Mining interface after updating the C++ Mining interface last commit.
Add interface changes from bitcoin#30409 bitcoin#30356 bitcoin#30440 bitcoin#30443 This commit updates the Cap'n Proto Mining interface after updating the C++ Mining interface last commit.
@ismaelsadeeq thanks, updated the description. |
post merge ACK |
Add interface changes from bitcoin#30409 bitcoin#30356 bitcoin#30440 bitcoin#30443 This commit changes the C++ Mining interface without changing the corresponding Capn'Proto interface. The capnp interface is updated the next commit.
Add interface changes from bitcoin#30409 bitcoin#30356 bitcoin#30440 bitcoin#30443 This commit updates the Cap'n Proto Mining interface after updating the C++ Mining interface last commit.
Add interface changes from bitcoin#30409 bitcoin#30356 bitcoin#30440 bitcoin#30443 This commit changes the C++ Mining interface without changing the corresponding Capn'Proto interface. The capnp interface is updated the next commit.
Add interface changes from bitcoin#30409 bitcoin#30356 bitcoin#30440 bitcoin#30443 This commit updates the Cap'n Proto Mining interface after updating the C++ Mining interface last commit.
Add interface changes from bitcoin#30409 bitcoin#30356 bitcoin#30440 bitcoin#30443 This commit updates the Cap'n Proto Mining interface after updating the C++ Mining interface last commit.
Add interface changes from bitcoin#30409 bitcoin#30356 bitcoin#30440 bitcoin#30443 This commit updates the Cap'n Proto Mining interface after updating the C++ Mining interface last commit.
Add interface changes from bitcoin#30409 bitcoin#30356 bitcoin#30440 bitcoin#30443 This commit changes the C++ Mining interface without changing the corresponding Capn'Proto interface. The capnp interface is updated the next commit.
Add interface changes from bitcoin#30409 bitcoin#30356 bitcoin#30440 bitcoin#30443 This commit updates the Cap'n Proto Mining interface after updating the C++ Mining interface last commit.
Add interface changes from bitcoin#30409 bitcoin#30356 bitcoin#30440 bitcoin#30443 This commit updates the Cap'n Proto Mining interface after updating the C++ Mining interface last commit.
Add interface changes from bitcoin#30409 bitcoin#30356 bitcoin#30440 bitcoin#30443 This commit updates the Cap'n Proto Mining interface after updating the C++ Mining interface last commit.
Add interface changes from bitcoin#30409 bitcoin#30356 bitcoin#30440 bitcoin#30443 This commit updates the Cap'n Proto Mining interface after updating the C++ Mining interface last commit.
Add interface changes from bitcoin#30409 bitcoin#30356 bitcoin#30440 bitcoin#30443
…nough to give up" size delta PR bitcoin#30356 incorrectly changed a constant of `4000` to `m_options.coinbase_max_additional_weight` in the check for when to give up finding another transaction to fill the block: ```diff if (nConsecutiveFailed > MAX_CONSECUTIVE_FAILURES && nBlockWeight > - m_options.nBlockMaxWeight - 4000) { + m_options.nBlockMaxWeight - m_options.block_reserved_weight) { // Give up if we're close to full and haven't succeeded in a while break; } ``` But this constant did not deal with the reserved weight at all. It was in fact simply checking if the block was close to full, and if so, giving up finding another transaction to pad it with after `MAX_CONSECUTIVE_FAILURES` failed attempts. It doesn't seem very logical to reuse the reserve size for this purpose, and it would be overcomplicated to add yet another setting, so this PR changes it to a new constexpr.
…nough to give up" weight delta PR bitcoin#30356 incorrectly changed a constant of `4000` to `m_options.coinbase_max_additional_weight` in the check for when to give up finding another transaction to fill the block: ```diff if (nConsecutiveFailed > MAX_CONSECUTIVE_FAILURES && nBlockWeight > - m_options.nBlockMaxWeight - 4000) { + m_options.nBlockMaxWeight - m_options.block_reserved_weight) { // Give up if we're close to full and haven't succeeded in a while break; } ``` But this constant did not deal with the reserved weight at all. It was in fact simply checking if the block was close to full, and if so, giving up finding another transaction to pad it with after `MAX_CONSECUTIVE_FAILURES` failed attempts. It doesn't seem very logical to reuse the reserve weight for this purpose, and it would be overcomplicated to add yet another setting, so this PR changes it to a new constexpr.
…lock is full enough to give up" weight delta 524f981 Bugfix: Miner: Don't reuse block_reserved_weight for "block is full enough to give up" weight delta (Luke Dashjr) Pull request description: PR #30356 incorrectly changed a constant of `4000` to `m_options.coinbase_max_additional_weight` in the check for when to give up finding another transaction to fill the block: ```diff if (nConsecutiveFailed > MAX_CONSECUTIVE_FAILURES && nBlockWeight > - m_options.nBlockMaxWeight - 4000) { + m_options.nBlockMaxWeight - m_options.block_reserved_weight) { // Give up if we're close to full and haven't succeeded in a while break; } ``` But this constant did not deal with the reserved weight at all. It was in fact simply checking if the block was close to full, and if so, giving up finding another transaction to pad it with after `MAX_CONSECUTIVE_FAILURES` failed attempts. It doesn't seem very logical to reuse the reserve weight for this purpose, and it would be overcomplicated to add yet another setting, so this PR changes it to a new constexpr. ACKs for top commit: achow101: ACK 524f981 darosior: utACK 524f981 ismaelsadeeq: ACK 524f981 Tree-SHA512: c066debc34a021380424bd21b40444071b736325e41779a41590c2c8a6822ceeaf910fe067817c1dba108210b24c574977b0350b29520502e7af79d3b405928b
…nough to give up" weight delta PR bitcoin#30356 incorrectly changed a constant of `4000` to `m_options.coinbase_max_additional_weight` in the check for when to give up finding another transaction to fill the block: ```diff if (nConsecutiveFailed > MAX_CONSECUTIVE_FAILURES && nBlockWeight > - m_options.nBlockMaxWeight - 4000) { + m_options.nBlockMaxWeight - m_options.block_reserved_weight) { // Give up if we're close to full and haven't succeeded in a while break; } ``` But this constant did not deal with the reserved weight at all. It was in fact simply checking if the block was close to full, and if so, giving up finding another transaction to pad it with after `MAX_CONSECUTIVE_FAILURES` failed attempts. It doesn't seem very logical to reuse the reserve weight for this purpose, and it would be overcomplicated to add yet another setting, so this PR changes it to a new constexpr.
When generating a block template through e.g. getblocktemplate RPC, we reserve 4000 weight units and 400 sigops. Pools use this space for their coinbase outputs.
At least one pool patched their Bitcoin Core node to adjust these hardcoded values. They eventually produced an invalid block which exceeded the sigops limit.
The existince of such patches suggests it may be useful to make this value configurable. This PR would make such a change easier. However, the main motivation is that in the Stratum v2 spec requires the pool to communicate the maximum bytes they intend
to add to the coinbase outputs.
Specifically the
CoinbaseOutputDataSize
message which is part of the Template Distribution Protocol has a fieldcoinbase_output_max_additional_size
.A proposed change to the spec adds the max additional sigops as well: stratum-mining/sv2-spec#86. Whether that change makes it into the spec is not important though, as adding both to
BlockAssembler::Options
makes sense.The first commit is a test refactor followup for #30335, related to the code that's changed here, but not required.
The second commit introduces BlockCreateOptions, with just
use_mempool
.The thirds commit adds
coinbase_max_additional_weight
andcoinbase_output_max_additional_sigops
toBlockCreateOptions
. They use the originally hardcoded values, and no existing caller overrides these defaults. This changes in #29432.