From 323cfed5959b25c98235ec988b408fc5e3391e3c Mon Sep 17 00:00:00 2001 From: Sjors Provoost Date: Fri, 28 Jun 2024 10:35:13 +0200 Subject: [PATCH 1/3] refactor: use CHECK_NONFATAL to avoid single-use symbol --- src/rpc/mining.cpp | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/src/rpc/mining.cpp b/src/rpc/mining.cpp index 7e420dcd9b5..e16f75ab781 100644 --- a/src/rpc/mining.cpp +++ b/src/rpc/mining.cpp @@ -778,9 +778,7 @@ static RPCHelpMan getblocktemplate() } ENTER_CRITICAL_SECTION(cs_main); - std::optional maybe_tip{miner.getTipHash()}; - CHECK_NONFATAL(maybe_tip); - tip = maybe_tip.value(); + tip = CHECK_NONFATAL(miner.getTipHash()).value(); if (!IsRPCRunning()) throw JSONRPCError(RPC_CLIENT_NOT_CONNECTED, "Shutting down"); From 6b4c817d4b978adf69738677c74855ef0675f333 Mon Sep 17 00:00:00 2001 From: Sjors Provoost Date: Tue, 16 Jul 2024 10:13:28 +0200 Subject: [PATCH 2/3] refactor: pass BlockCreateOptions to createNewBlock Rather than pass options individually to createNewBlock and then combining them into BlockAssembler::Options, this commit introduces BlockCreateOptions and passes that instead. Currently there's only one option (use_mempool) but the next commit adds more. Co-authored-by: Ryan Ofsky --- src/interfaces/mining.h | 8 +++++--- src/node/interfaces.cpp | 9 ++++----- src/node/miner.cpp | 2 +- src/node/miner.h | 3 ++- src/node/types.h | 9 +++++++++ src/rpc/mining.cpp | 2 +- 6 files changed, 22 insertions(+), 11 deletions(-) diff --git a/src/interfaces/mining.h b/src/interfaces/mining.h index 974490561a4..cebe97edb7a 100644 --- a/src/interfaces/mining.h +++ b/src/interfaces/mining.h @@ -5,9 +5,11 @@ #ifndef BITCOIN_INTERFACES_MINING_H #define BITCOIN_INTERFACES_MINING_H +#include +#include + #include #include -#include namespace node { struct CBlockTemplate; @@ -41,10 +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] options options for creating the block * @returns a block template */ - virtual std::unique_ptr createNewBlock(const CScript& script_pub_key, bool use_mempool = true) = 0; + virtual std::unique_ptr createNewBlock(const CScript& script_pub_key, const node::BlockCreateOptions& options={}) = 0; /** * Processes new block. A valid new block is automatically relayed to peers. diff --git a/src/node/interfaces.cpp b/src/node/interfaces.cpp index ef12ffe34b0..46d36f83f84 100644 --- a/src/node/interfaces.cpp +++ b/src/node/interfaces.cpp @@ -884,12 +884,11 @@ public: return TestBlockValidity(state, chainman().GetParams(), chainman().ActiveChainstate(), block, tip, /*fCheckPOW=*/false, check_merkle_root); } - std::unique_ptr createNewBlock(const CScript& script_pub_key, bool use_mempool) override + std::unique_ptr createNewBlock(const CScript& script_pub_key, const BlockCreateOptions& options) override { - BlockAssembler::Options options; - ApplyArgsManOptions(gArgs, options); - - 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; } diff --git a/src/node/miner.cpp b/src/node/miner.cpp index 291f1d5fc7f..67465b1e706 100644 --- a/src/node/miner.cpp +++ b/src/node/miner.cpp @@ -66,7 +66,7 @@ static BlockAssembler::Options ClampOptions(BlockAssembler::Options 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)} { diff --git a/src/node/miner.h b/src/node/miner.h index 622ca16c8fd..efd773eb310 100644 --- 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 #include #include #include @@ -153,7 +154,7 @@ 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}; diff --git a/src/node/types.h b/src/node/types.h index 0461e85f430..d4e7d930152 100644 --- 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 + namespace node { enum class TransactionError { OK, //!< No error @@ -24,6 +26,13 @@ enum class TransactionError { MAX_BURN_EXCEEDED, INVALID_PACKAGE, }; + +struct BlockCreateOptions { + /** + * Set false to omit mempool transactions + */ + bool use_mempool{true}; +}; } // namespace node #endif // BITCOIN_NODE_TYPES_H diff --git a/src/rpc/mining.cpp b/src/rpc/mining.cpp index e16f75ab781..8482ce6eb2e 100644 --- a/src/rpc/mining.cpp +++ b/src/rpc/mining.cpp @@ -371,7 +371,7 @@ static RPCHelpMan generateblock() ChainstateManager& chainman = EnsureChainman(node); { - std::unique_ptr blocktemplate{miner.createNewBlock(coinbase_script, /*use_mempool=*/false)}; + std::unique_ptr blocktemplate{miner.createNewBlock(coinbase_script, {.use_mempool = false})}; if (!blocktemplate) { throw JSONRPCError(RPC_INTERNAL_ERROR, "Couldn't create new block"); } From c504b6997b1acc9771ad1f52efaa4be2b4966c6c Mon Sep 17 00:00:00 2001 From: Sjors Provoost Date: Wed, 17 Jul 2024 18:33:15 +0200 Subject: [PATCH 3/3] refactor: add coinbase constraints to BlockCreateOptions 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. https://bitcoin.stackexchange.com/questions/117837/how-many-sigops-are-in-the-invalid-block-783426 The existince of such patches suggests it may be useful to make this value configurable. This commit would make such a change easier. The main motivation however is that the Stratum v2 spec requires the pool to communicate the maximum bytes they intend to add to the coinbase outputs. A proposed change to the spec would also require them to communicate the maximum number of sigops. This commit also documents what happens when -blockmaxweight is lower than the coinbase reserved value. Co-authored-by: Ryan Ofsky --- src/node/miner.cpp | 13 ++++++++----- src/node/types.h | 11 +++++++++++ 2 files changed, 19 insertions(+), 5 deletions(-) diff --git a/src/node/miner.cpp b/src/node/miner.cpp index 67465b1e706..fa2d979b86a 100644 --- a/src/node/miner.cpp +++ b/src/node/miner.cpp @@ -59,8 +59,11 @@ void RegenerateCommitments(CBlock& block, ChainstateManager& chainman) static BlockAssembler::Options ClampOptions(BlockAssembler::Options options) { - // Limit weight to between 4K and DEFAULT_BLOCK_MAX_WEIGHT for sanity: - options.nBlockMaxWeight = std::clamp(options.nBlockMaxWeight, 4000, DEFAULT_BLOCK_MAX_WEIGHT); + Assert(options.coinbase_max_additional_weight <= DEFAULT_BLOCK_MAX_WEIGHT); + Assert(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(options.nBlockMaxWeight, options.coinbase_max_additional_weight, DEFAULT_BLOCK_MAX_WEIGHT); return options; } @@ -87,8 +90,8 @@ void BlockAssembler::resetBlock() inBlock.clear(); // Reserve space for coinbase tx - nBlockWeight = 4000; - nBlockSigOpsCost = 400; + nBlockWeight = m_options.coinbase_max_additional_weight; + nBlockSigOpsCost = m_options.coinbase_output_max_additional_sigops; // These counters do not include coinbase tx nBlockTx = 0; @@ -379,7 +382,7 @@ void BlockAssembler::addPackageTxs(const CTxMemPool& mempool, int& nPackagesSele ++nConsecutiveFailed; if (nConsecutiveFailed > MAX_CONSECUTIVE_FAILURES && nBlockWeight > - m_options.nBlockMaxWeight - 4000) { + m_options.nBlockMaxWeight - m_options.coinbase_max_additional_weight) { // Give up if we're close to full and haven't succeeded in a while break; } diff --git a/src/node/types.h b/src/node/types.h index d4e7d930152..bb8d17ef43f 100644 --- a/src/node/types.h +++ b/src/node/types.h @@ -32,6 +32,17 @@ 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