mining: fix -blockreservedweight shadows IPC option

Sjors commented at 1:38 pm on November 28, 2025: member

Also enforce MINIMUM_BLOCK_RESERVED_WEIGHT for IPC clients.

The -blockreservedweight startup option should only affect RPC code, because IPC clients (currently) do not have a way to signal their intent to use the node default (the BlockCreateOptions struct defaults merely document a recommendation for client software).

Before this PR however, if the user set -blockreservedweight then ApplyArgsManOptions would cause the block_reserved_weight option passed by IPC clients to be ignored. Users who don’t set this value were not affected.

Fix this by making BlockCreateOptions::block_reserved_weight an std::optional.

Internal interface users, such as the RPC call sites, don’t set a value so -blockreservedweight is used. Whereas IPC clients do set a value which is no longer ignored.

Test coverage is added, with a preliminary commit that refactors the create_block_template and wait_next_template helpers.

mining_basic.py already ensured -blockreservedweight is enforced by mining RPC methods. The second commit adds coverage for Mining interface IPC clients. It also verifies that -blockreservedweight has no effect on them.

The third commit enforces MINIMUM_BLOCK_RESERVED_WEIGHT for IPC clients. Previously lower values were quietly clamped.

DrahtBot added the label Mining on Nov 28, 2025

DrahtBot commented at 1:38 pm on November 28, 2025: contributor

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

Reviews

See the guideline for information on the review process.

Type	Reviewers
Stale ACK	ryanofsky

If your review is incorrectly listed, please copy-paste <!–meta-tag:bot-skip–> into the comment that the bot should ignore.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#34184 (mining: add cooldown to createNewBlock() immediately after IBD by Sjors)
#33966 (refactor: disentangle miner startup defaults from runtime options by Sjors)
#33936 (mining: pass missing context to createNewBlock() and checkBlock() by Sjors)
#33922 (mining: add getMemoryLoad() and track template non-mempool memory footprint by Sjors)
#33819 (mining: getCoinbase() returns struct instead of raw tx by Sjors)
#33795 (test: Ignore error message give from python because of PYTHON_GIL by kevkevinpal)
#32420 (miner: drop dummy extraNonce in coinbase scriptSig for templates requested via IPC by Sjors)

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 force-pushed on Nov 28, 2025

DrahtBot added the label CI failed on Nov 28, 2025

DrahtBot commented at 1:49 pm on November 28, 2025: contributor

🚧 At least one of the CI tasks failed. Task i686, no IPC: https://github.com/bitcoin/bitcoin/actions/runs/19765451883/job/56637015664 LLM reason (✨ experimental): Compile error: ISO C++ reorder-init-list warnings promoted to errors in src/rpc/mining.cpp, causing CI failure.

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.

in src/rpc/mining.cpp:882 in 46d901c253

877@@ -871,7 +878,9 @@ static RPCHelpMan getblocktemplate()
878         time_start = GetTime();
879 
880         // Create new block
881-        block_template = miner.createNewBlock();
882+        block_template = miner.createNewBlock({
883+            .block_reserved_weight = size_t(node.args->GetIntArg("-blockreservedweight", DEFAULT_BLOCK_RESERVED_WEIGHT))

Sjors commented at 1:52 pm on November 28, 2025:

The big refactor in #33966 simplifies this to node.mining_args.default_block_reserved_weight. But using GetIntArg directly here is easier to backport.

Sjors commented at 10:42 am on December 2, 2025:

The latest push leaves RPC code untouched.

DrahtBot removed the label CI failed on Nov 28, 2025

ryanofsky commented at 8:22 pm on December 1, 2025: contributor

Concept ACK. Would suggest updating -blockreservedweight documentation to say it only affects RPC mining clients, not IPC clients.

I think implementation of this could be improved a bit, depending on if you think it would be useful for IPC clients to “have a way to signal their intent to use the node default”?

If yes, would suggest just making the field std::optional:

 0--- a/src/node/miner.cpp
 1+++ b/src/node/miner.cpp
 2@@ -78,11 +78,11 @@ void RegenerateCommitments(CBlock& block, ChainstateManager& chainman)
 3 
 4 static BlockAssembler::Options ClampOptions(BlockAssembler::Options options)
 5 {
 6-    options.block_reserved_weight = std::clamp<size_t>(options.block_reserved_weight, MINIMUM_BLOCK_RESERVED_WEIGHT, MAX_BLOCK_WEIGHT);
 7+    options.block_reserved_weight = std::clamp<size_t>(options.block_reserved_weight.value_or(DEFAULT_BLOCK_RESERVED_WEIGHT), MINIMUM_BLOCK_RESERVED_WEIGHT, MAX_BLOCK_WEIGHT);
 8     options.coinbase_output_max_additional_sigops = std::clamp<size_t>(options.coinbase_output_max_additional_sigops, 0, MAX_BLOCK_SIGOPS_COST);
 9     // Limit weight to between block_reserved_weight and MAX_BLOCK_WEIGHT for sanity:
10     // block_reserved_weight can safely exceed -blockmaxweight, but the rest of the block template will be empty.
11-    options.nBlockMaxWeight = std::clamp<size_t>(options.nBlockMaxWeight, options.block_reserved_weight, MAX_BLOCK_WEIGHT);
12+    options.nBlockMaxWeight = std::clamp<size_t>(options.nBlockMaxWeight, *options.block_reserved_weight, MAX_BLOCK_WEIGHT);
13     return options;
14 }
15 
16@@ -102,13 +102,15 @@ void ApplyArgsManOptions(const ArgsManager& args, BlockAssembler::Options& optio
17         if (const auto parsed{ParseMoney(*blockmintxfee)}) options.blockMinFeeRate = CFeeRate{*parsed};
18     }
19     options.print_modified_fee = args.GetBoolArg("-printpriority", options.print_modified_fee);
20-    options.block_reserved_weight = args.GetIntArg("-blockreservedweight", options.block_reserved_weight);
21+    if (!options.block_reserved_weight) {
22+        options.block_reserved_weight = args.GetIntArg("-blockreservedweight");
23+    }
24 }
25 
26 void BlockAssembler::resetBlock()
27 {
28     // Reserve space for fixed-size block header, txs count, and coinbase tx.
29-    nBlockWeight = m_options.block_reserved_weight;
30+    nBlockWeight = *m_options.block_reserved_weight;
31     nBlockSigOpsCost = m_options.coinbase_output_max_additional_sigops;
32 
33     // These counters do not include coinbase tx
34--- a/src/node/types.h
35+++ b/src/node/types.h
36@@ -42,7 +42,7 @@ struct BlockCreateOptions {
37      * The default reserved weight for the fixed-size block header,
38      * transaction count and coinbase transaction.
39      */
40-    size_t block_reserved_weight{DEFAULT_BLOCK_RESERVED_WEIGHT};
41+    std::optional<size_t> block_reserved_weight{};
42     /**
43      * The maximum additional sigops which the pool will add in coinbase
44      * transaction outputs.

If no, would suggest having separate functions to read options which IPC clients can and can’t control:

  0--- a/src/node/interfaces.cpp
  1+++ b/src/node/interfaces.cpp
  2@@ -963,8 +963,12 @@ public:
  3         // Ensure m_tip_block is set so consumers of BlockTemplate can rely on that.
  4         if (!waitTipChanged(uint256::ZERO, MillisecondsDouble::max())) return {};
  5 
  6+        // Construct block assembler options from block create options and
  7+        // ArgsManager preferences. Intentionally do not call
  8+        // ReadBlockCreateArgs, so IPC create options will take precedence over
  9+        // ArgsManager values.
 10         BlockAssembler::Options assemble_options{options};
 11-        ApplyArgsManOptions(*Assert(m_node.args), assemble_options);
 12+        ReadBlockAssemblerArgs(*Assert(m_node.args), assemble_options);
 13         return std::make_unique<BlockTemplateImpl>(assemble_options, BlockAssembler{chainman().ActiveChainstate(), context()->mempool.get(), assemble_options}.CreateNewBlock(), m_node);
 14     }
 15 
 16--- a/src/node/miner.cpp
 17+++ b/src/node/miner.cpp
 18@@ -94,7 +94,7 @@ BlockAssembler::BlockAssembler(Chainstate& chainstate, const CTxMemPool* mempool
 19 {
 20 }
 21 
 22-void ApplyArgsManOptions(const ArgsManager& args, BlockAssembler::Options& options)
 23+void ReadBlockAssemblerArgs(const ArgsManager& args, BlockAssembler::Options& options)
 24 {
 25     // Block resource limits
 26     options.nBlockMaxWeight = args.GetIntArg("-blockmaxweight", options.nBlockMaxWeight);
 27@@ -102,6 +102,10 @@ void ApplyArgsManOptions(const ArgsManager& args, BlockAssembler::Options& optio
 28         if (const auto parsed{ParseMoney(*blockmintxfee)}) options.blockMinFeeRate = CFeeRate{*parsed};
 29     }
 30     options.print_modified_fee = args.GetBoolArg("-printpriority", options.print_modified_fee);
 31+}
 32+
 33+void ReadBlockCreateArgs(const ArgsManager& args, BlockCreateOptions& options)
 34+{
 35     options.block_reserved_weight = args.GetIntArg("-blockreservedweight", options.block_reserved_weight);
 36 }
 37 
 38--- a/src/node/miner.h
 39+++ b/src/node/miner.h
 40@@ -131,8 +131,11 @@ int64_t UpdateTime(CBlockHeader* pblock, const Consensus::Params& consensusParam
 41 /** Update an old GenerateCoinbaseCommitment from CreateNewBlock after the block txs have changed */
 42 void RegenerateCommitments(CBlock& block, ChainstateManager& chainman);
 43 
 44+/** Apply -blockreservedweight options from ArgsManager to BlockCreateOptions. */
 45+void ReadBlockCreateArgs(const ArgsManager& args, BlockCreateOptions& options);
 46+
 47 /** Apply -blockmintxfee and -blockmaxweight options from ArgsManager to BlockAssembler options. */
 48-void ApplyArgsManOptions(const ArgsManager& gArgs, BlockAssembler::Options& options);
 49+void ReadBlockAssemblerArgs(const ArgsManager& args, BlockAssembler::Options& options);
 50 
 51 /* Compute the block's merkle root, insert or replace the coinbase transaction and the merkle root into the block */
 52 void AddMerkleRootAndCoinbase(CBlock& block, CTransactionRef coinbase, uint32_t version, uint32_t timestamp, uint32_t nonce);
 53--- a/src/rpc/mining.cpp
 54+++ b/src/rpc/mining.cpp
 55@@ -161,11 +161,13 @@ static bool GenerateBlock(ChainstateManager& chainman, CBlock&& block, uint64_t&
 56     return true;
 57 }
 58 
 59-static UniValue generateBlocks(ChainstateManager& chainman, Mining& miner, const CScript& coinbase_output_script, int nGenerate, uint64_t nMaxTries)
 60+static UniValue generateBlocks(ChainstateManager& chainman, ArgsManager& args, Mining& miner, const CScript& coinbase_output_script, int nGenerate, uint64_t nMaxTries)
 61 {
 62     UniValue blockHashes(UniValue::VARR);
 63+    node::BlockCreateOptions options{ .coinbase_output_script = coinbase_output_script };
 64+    ReadBlockCreateArgs(args, options);
 65     while (nGenerate > 0 && !chainman.m_interrupt) {
 66-        std::unique_ptr<BlockTemplate> block_template(miner.createNewBlock({ .coinbase_output_script = coinbase_output_script }));
 67+        std::unique_ptr<BlockTemplate> block_template(miner.createNewBlock(options));
 68         CHECK_NONFATAL(block_template);
 69 
 70         std::shared_ptr<const CBlock> block_out;
 71@@ -247,9 +249,10 @@ static RPCHelpMan generatetodescriptor()
 72 
 73     NodeContext& node = EnsureAnyNodeContext(request.context);
 74     Mining& miner = EnsureMining(node);
 75+    ArgsManager& args = EnsureArgsman(node);
 76     ChainstateManager& chainman = EnsureChainman(node);
 77 
 78-    return generateBlocks(chainman, miner, coinbase_output_script, num_blocks, max_tries);
 79+    return generateBlocks(chainman, args, miner, coinbase_output_script, num_blocks, max_tries);
 80 },
 81     };
 82 }
 83@@ -293,11 +296,12 @@ static RPCHelpMan generatetoaddress()
 84 
 85     NodeContext& node = EnsureAnyNodeContext(request.context);
 86     Mining& miner = EnsureMining(node);
 87+    ArgsManager& args = EnsureArgsman(node);
 88     ChainstateManager& chainman = EnsureChainman(node);
 89 
 90     CScript coinbase_output_script = GetScriptForDestination(destination);
 91 
 92-    return generateBlocks(chainman, miner, coinbase_output_script, num_blocks, max_tries);
 93+    return generateBlocks(chainman, args, miner, coinbase_output_script, num_blocks, max_tries);
 94 },
 95     };
 96 }
 97@@ -345,6 +349,7 @@ static RPCHelpMan generateblock()
 98 
 99     NodeContext& node = EnsureAnyNodeContext(request.context);
100     Mining& miner = EnsureMining(node);
101+    ArgsManager& args = EnsureArgsman(node);
102     const CTxMemPool& mempool = EnsureMemPool(node);
103 
104     std::vector<CTransactionRef> txs;
105@@ -374,9 +379,11 @@ static RPCHelpMan generateblock()
106 
107     ChainstateManager& chainman = EnsureChainman(node);
108     {
109+        node::BlockCreateOptions options{.use_mempool = false, .coinbase_output_script = coinbase_output_script};
110+        ReadBlockCreateArgs(args, options);
111         LOCK(chainman.GetMutex());
112         {
113-            std::unique_ptr<BlockTemplate> block_template{miner.createNewBlock({.use_mempool = false, .coinbase_output_script = coinbase_output_script})};
114+            std::unique_ptr<BlockTemplate> block_template{miner.createNewBlock(options)};
115             CHECK_NONFATAL(block_template);
116 
117             block = block_template->getBlock();
118@@ -471,7 +478,8 @@ static RPCHelpMan getmininginfo()
119     obj.pushKV("networkhashps",    getnetworkhashps().HandleRequest(request));
120     obj.pushKV("pooledtx",         (uint64_t)mempool.size());
121     BlockAssembler::Options assembler_options;
122-    ApplyArgsManOptions(*node.args, assembler_options);
123+    ReadBlockCreateArgs(*node.args, assembler_options);
124+    ReadBlockAssemblerArgs(*node.args, assembler_options);
125     obj.pushKV("blockmintxfee", ValueFromAmount(assembler_options.blockMinFeeRate.GetFeePerK()));
126     obj.pushKV("chain", chainman.GetParams().GetChainTypeString());
127 
128@@ -871,7 +879,10 @@ static RPCHelpMan getblocktemplate()
129         time_start = GetTime();
130 
131         // Create new block
132-        block_template = miner.createNewBlock();
133+        ArgsManager& args = EnsureArgsman(node);
134+        node::BlockCreateOptions options;
135+        ReadBlockCreateArgs(args, options);
136+        block_template = miner.createNewBlock(options);
137         CHECK_NONFATAL(block_template);
138 
139 
140--- a/src/test/util/mining.cpp
141+++ b/src/test/util/mining.cpp
142@@ -138,6 +138,7 @@ std::shared_ptr<CBlock> PrepareBlock(const NodeContext& node, const CScript& coi
143 {
144     BlockAssembler::Options assembler_options;
145     assembler_options.coinbase_output_script = coinbase_scriptPubKey;
146-    ApplyArgsManOptions(*node.args, assembler_options);
147+    ReadBlockCreateArgs(*node.args, assembler_options);
148+    ReadBlockAssemblerArgs(*node.args, assembler_options);
149     return PrepareBlock(node, assembler_options);
150 }

The current implementation 46d901c253107ffba14ddce6c3154b46bd471fc2 seems a little less ideal using repeated GetIntArg calls instead a function to read the options.

Sjors force-pushed on Dec 2, 2025

Sjors commented at 10:41 am on December 2, 2025: member

if you think it would be useful for IPC clients to “have a way to signal their intent to use the node default”

sv2-tp always sets the value (based on coinbase_output_max_additional_size in CoinbaseOutputConstraints), so it wouldn’t benefit. But it does seem better to allow clients to omit this value.

For that to work however, we’d also need to change mining.capnp, e.g.:

0blockReservedWeight [@1](/bitcoin-bitcoin/contributor/1/) :UInt64 $Proxy.name("block_reserved_weight");
1hasBlockReservedWeight [@3](/bitcoin-bitcoin/contributor/3/) :Bool = true $Proxy.skip;

Adding the has field and having it default to true means it’s not a breaking change. But this doesn’t actually work :-)

Also, if we really wanted to make this optional for IPC clients, I would prefer to add direct support for optionals to capnp files, as a breaking change.

However, I did take your patch because it allows this PR to simplified:

internal callers will see coinbase_output_max_additional_size as optional and can omit it. That lets me avoid the four GetIntArg calls (and leave rpc/mining.cpp alone).
IPC callers don’t see it as optional and always provide a value (which we can change later)

I added documentation to make this behavior clear.

DrahtBot added the label CI failed on Dec 2, 2025

Sjors force-pushed on Dec 2, 2025

Sjors commented at 2:25 pm on December 2, 2025: member

Rebased after #33591 because #33966 has a trivial merge conflict with it and I want to keep that cleanly based on this PR.

DrahtBot removed the label CI failed on Dec 2, 2025

in src/node/miner.cpp:84 in 1f2f4ab9d3 outdated

77@@ -78,11 +78,14 @@ void RegenerateCommitments(CBlock& block, ChainstateManager& chainman)
78 
79 static BlockAssembler::Options ClampOptions(BlockAssembler::Options options)
80 {
81-    options.block_reserved_weight = std::clamp<size_t>(options.block_reserved_weight, MINIMUM_BLOCK_RESERVED_WEIGHT, MAX_BLOCK_WEIGHT);
82+    // Typically block_reserved_weight is set by ApplyArgsManOptions before
83+    // the constructor calls this; value_or(DEFAULT_BLOCK_RESERVED_WEIGHT) only
84+    // affects (test) call sites that don't go through the Mining interface.
85+    options.block_reserved_weight = std::clamp<size_t>(options.block_reserved_weight.value_or(DEFAULT_BLOCK_RESERVED_WEIGHT), MINIMUM_BLOCK_RESERVED_WEIGHT, MAX_BLOCK_WEIGHT);

ryanofsky commented at 3:57 pm on December 2, 2025:

In commit “mining: add getCoinbase()” (73553c809ef6517839c4d1b02b657e6fa401f8a8)

Is there any particular reason to apply DEFAULT_BLOCK_RESERVED_WEIGHT in ApplyArgsManOptions? It would seem better to leave the option unset if the argument is unset, and only apply the default one place instead of two.

Would suggest:

 0--- a/src/node/miner.cpp
 1+++ b/src/node/miner.cpp
 2@@ -78,9 +78,6 @@ void RegenerateCommitments(CBlock& block, ChainstateManager& chainman)
 3 
 4 static BlockAssembler::Options ClampOptions(BlockAssembler::Options options)
 5 {
 6-    // Typically block_reserved_weight is set by ApplyArgsManOptions before
 7-    // the constructor calls this; value_or(DEFAULT_BLOCK_RESERVED_WEIGHT) only
 8-    // affects (test) call sites that don't go through the Mining interface.
 9     options.block_reserved_weight = std::clamp<size_t>(options.block_reserved_weight.value_or(DEFAULT_BLOCK_RESERVED_WEIGHT), MINIMUM_BLOCK_RESERVED_WEIGHT, MAX_BLOCK_WEIGHT);
10     options.coinbase_output_max_additional_sigops = std::clamp<size_t>(options.coinbase_output_max_additional_sigops, 0, MAX_BLOCK_SIGOPS_COST);
11     // Limit weight to between block_reserved_weight and MAX_BLOCK_WEIGHT for sanity:
12@@ -106,7 +103,7 @@ void ApplyArgsManOptions(const ArgsManager& args, BlockAssembler::Options& optio
13     }
14     options.print_modified_fee = args.GetBoolArg("-printpriority", options.print_modified_fee);
15     if (!options.block_reserved_weight) {
16-        options.block_reserved_weight = args.GetIntArg("-blockreservedweight", DEFAULT_BLOCK_RESERVED_WEIGHT);
17+        options.block_reserved_weight = args.GetIntArg("-blockreservedweight");
18     }
19 }
20

Sjors commented at 6:45 pm on December 2, 2025:

Since -blockreservedweight has a default value, I don’t think the absence of this setting should behave any different from when the user manually sets it to the default. This makes that intention clear.

It also makes the transition to ApplyMiningDefaults in https://github.com/bitcoin/bitcoin/pull/33966/commits/5f3e67efddd2a8a8743bb19765a5b38755d3f460 simpler, because default_block_max_weight and default_block_reserved_weight on MiningArgs are not optionals. That’s for the same reason: both -blockreservedweight and -maxblockweight have a default value.

ryanofsky commented at 5:52 pm on December 3, 2025:

re: #33965 (review)

Yes that makes sense. I completely agree MiningArgs in #33966 should not use std::optional and that PR is correctly applying DEFAULT_BLOCK_RESERVED_WEIGHT in one place at the ArgsMan level, not the mining interface level.

I just don’t think the implementation of this PR should be muddled in anticipation of #33966 before MiningArgs is introduced. In this PR, it makes sense to apply DEFAULT_BLOCK_RESERVED_WEIGHT at the mining interface level, not the ArgsMan level, so it is applied once instead of twice, and there are not separate test-only and nontest code paths. I also think that nullopt is a better way to represent an unset setting in a field that is std::optional.

So I still think suggested diff above would be a nice simplification for this PR. Not important though, since #33966 should clean everything up in the end.

Sjors commented at 12:53 pm on December 4, 2025:

Ok, done and I rebased #33966 on that change.

in src/node/types.h:46 in 1f2f4ab9d3

41@@ -41,8 +42,10 @@ struct BlockCreateOptions {
42     /**
43      * The default reserved weight for the fixed-size block header,
44      * transaction count and coinbase transaction.
45+     *
46+     * Not optional for IPC clients.

ryanofsky commented at 4:04 pm on December 2, 2025:

In commit “mining: fix -blockreservedweight shadows IPC option” (1f2f4ab9d3fbdd3071810fa78a17563df3f56f91)

Might be good to clarify this to say something like “Cap’n Proto IPC clients do not currently have a way of leaving this field unset and will always provide a value.”

Otherwise it sounds likes requiring a value is actually intentional. Also documenting an optional field as not optional sounds contradictory and is potentially confusing.

Sjors commented at 6:48 pm on December 2, 2025:

I took the suggested message.

ryanofsky approved

ryanofsky commented at 5:08 pm on December 2, 2025: contributor

Code review ACK 1f2f4ab9d3fbdd3071810fa78a17563df3f56f91. This is a pretty clean way of letting the IPC reserved weight value take precedence over the -blockreservedweight, and a good first step towards making the IPC value optional in the future.

re: #33965 (comment)

For that to work however, we’d also need to change mining.capnp, e.g.:

Yes, sorry, I should have mentioned that. I think it makes sense to add support in the c++ implementation first, and the capn’proto interface later. There are many ways the capnproto interface could be modified to support this and be backwards compatible if desired. I think the nicest way to support making the value optional might be to use a union like:

0blockReservedWeight : union { none [@1](/bitcoin-bitcoin/contributor/1/) :Void; value [@2](/bitcoin-bitcoin/contributor/2/) :UInt64; };

(Interestingly according to https://capnproto.org/language.html#unions, you can even convert a non-union field into a union field without breaking backwards compatibility, as long as the old field is the lowest-numbered field in the union. However, this numbering would also affect the union default value so probably not worth it.)

Another way to support making the value optional would be to add a boolean has field like you suggested. Another way would be to “box” the value, putting it in a struct with a single field. Probably all approaches will require modifying libmultiprocess somehow, so would be better to implement in a separate PR.

Sjors force-pushed on Dec 2, 2025

in test/functional/interface_ipc.py:67 in 4d686142e0

62@@ -57,6 +63,9 @@ def set_test_params(self):
63 
64     def setup_nodes(self):
65         self.extra_init = [{"ipcbind": True}, {}]
66+        # The -blockreservedweight startup option has no effect on IPC clients.
67+        # Templates generated via RPC will be empty.

ryanofsky commented at 5:59 pm on December 3, 2025:

In commit “mining: fix -blockreservedweight shadows IPC option” (4d686142e079765cd31851481deb70659a9e9376)

Maybe update the comment to say this is being set to a high value to confirm that the -blockreservedweight option has no effect (IIUC). Current comment raises question of why the test is going out of its way to set an option that has no effect.

in test/functional/interface_ipc.py:225 in 4d686142e0

220@@ -210,6 +221,13 @@ async def interrupt_wait():
221             await interrupt_task
222             assert_equal(result.to_dict(), {})
223 
224+            # Use absurdly large reserved weight to force an empty template
225+            opts.blockReservedWeight = 4000000

ryanofsky commented at 6:17 pm on December 3, 2025:

In commit “mining: fix -blockreservedweight shadows IPC option” (4d686142e079765cd31851481deb70659a9e9376)

Use MAX_BLOCK_WEIGHT constant?

in test/functional/interface_ipc.py:183 in 4d686142e0 outdated

178@@ -170,6 +179,8 @@ async def async_routine():
179             waitnext = template2.result.waitNext(ctx, waitoptions)
180             miniwallet.send_self_transfer(fee_rate=10, from_node=self.nodes[0])
181             template4 = await waitnext
182+            # Ensure it did not timeout
183+            assert_not_equal(template4.to_dict(), {})

ryanofsky commented at 6:20 pm on December 3, 2025:

In commit “mining: fix -blockreservedweight shadows IPC option” (4d686142e079765cd31851481deb70659a9e9376)

Note: I guess if it did timeout you would get a confusing server error about trying to call a method on a null capability, so this error should be clearer

ryanofsky approved

ryanofsky commented at 6:23 pm on December 3, 2025: contributor

Code review ACK 4d686142e079765cd31851481deb70659a9e9376. Just updated a comment since last review. Still looks good! Only minor comments below, not important

Sjors force-pushed on Dec 4, 2025

ryanofsky approved

ryanofsky commented at 5:10 pm on December 4, 2025: contributor

Code review ACK 54d3054db129d4dd5aaf9faa3f6e6922159059b0. Just some more review suggestions applied since last review. This is a pretty clean fix preventing the -blockreservedweight configuration value from overriding the IPC requested value.

The followup #33966 will make the code merging IPC options and configuration options more clear. And a different followup could give IPC clients ability to use the -blockreservedweight value instead of needing to provide their own value.

DrahtBot added the label Needs rebase on Dec 16, 2025

Sjors force-pushed on Dec 19, 2025

Sjors commented at 7:49 am on December 19, 2025: member

Rebased after #34003. I added an initial test refactor commit to have the {create_block,wait_next}_template helpers introduced in that PR to return None if there’s a timeout or failure.

I also added a commit to enforce (rather than clamp) the minimum reserved block value, as noticed by @plebhash in https://github.com/stratum-mining/sv2-tp/pull/51#issuecomment-3670613634. I could make that it’s own PR but it’s rather trivial.

DrahtBot removed the label Needs rebase on Dec 19, 2025

in src/node/interfaces.cpp:966 in 6d7c3ee3fa

962@@ -963,6 +963,11 @@ class MinerImpl : public Mining
963 
964     std::unique_ptr<BlockTemplate> createNewBlock(const BlockCreateOptions& options) override
965     {
966+        // Enfore minimum block_reserved_weight to avoid silently clamping it

maflcko commented at 8:22 am on December 19, 2025:

from the LLM:

Enfore -> Enforce [misspelling in comment "Enfore minimum block_reserved_weight..." makes the word invalid]
wait -> weight [comment "Enforce minimum reserved wait for IPC clients too" should say "reserved weight" to match intended meaning]

Sjors commented at 9:01 am on December 19, 2025:

How many LLM’s does it take to fix a typo? :-)

Sjors force-pushed on Dec 19, 2025

Sjors requested review from ryanofsky on Dec 30, 2025

Sjors commented at 2:53 am on December 30, 2025: member

@plebhash maybe you can you give this a quick test against the SRI integration? Without this PR, if you set -blockreservedweigh it will ignore the corresponding IPC argument.

plebhash commented at 11:10 pm on December 30, 2025: none

@plebhash maybe you can you give this a quick test against the SRI integration? Without this PR, if you set -blockreservedweigh it will ignore the corresponding IPC argument.

what methodology would you suggest for this?

I assume we want to mine a block that’s filled up to its capacity, and then check the consumed weight to see how far it is from the consensus limit, right?

I’m trying to leverage integration_tests_sv2 crate, which allows us to write some Rust code to interact with a regtest node

but I’m having a really hard time to reliably populate the mempool with enough transactions to fill an entire block (at least in a test that wouldn’t take hours to execute)

Sjors commented at 2:39 am on December 31, 2025: member

@plebhash in the functional test for this PR I simply set -blockreservedweight=4000000. That should result in empty blocks. Then just use a normal value in the IPC calls and the blocks will still be empty (before this PR).

plebhash commented at 5:00 pm on December 31, 2025: none

ok I wrote an Integration Test with the following methodology:

Bitcoin Core in regtest mode with the following variations
- v30 vs this PR
- with vs without -blockreservedweight=4000000
Pool (which connects to Bitcoin Core via IPC)
Sniffer (between Pool and tProxy)
tProxy
Sv1 CPU miner

we loop while creating mempool transactions and monitoring the Extended Jobs sent by Pool

the test only exits once we see an Extended Job with merkle_path.len() > 0 (✅)… if the test never returns (❌), we conclude that all templates sent by Bitcoin Core are empty

note: Pool originally generates CoinbaseOutputConstraints.coinbase_output_max_additional_size = 31

but internally, its BitcoinCoreSv2 abstraction multiplies it by 4 and then clamps the result to 2000 while crafting the CreateNewBlock IPC request… so I also added a variation on the tests with and without clamping this to 2000

here are the results:

WITH clamping 4 * CoinbaseOutputConstraints.coinbase_output_max_additional_size to 2000 while sending CreateNewBlock:

	v30	#33965
WITH `-blockreservedweight=4000000`	only sends EMPTY templates ❌	sends NON-EMPTY template ✅
WITHOUT `-blockreservedweight=4000000`	sends NON-EMPTY template ✅	sends NON-EMPTY template ✅

WITHOUT clamping 4 * CoinbaseOutputConstraints.coinbase_output_max_additional_size to 2000 while sending CreateNewBlock:

	v30	#33965
with `-blockreservedweight=4000000`	only sends EMPTY templates ❌	`CreateNewBlock` fails 💥
without `-blockreservedweight=4000000`	sends NON-EMPTY template ✅	`CreateNewBlock` fails 💥

both errors for CreateNewBlock were:

02025-12-31T16:42:09.307986Z ERROR bitcoin_core_sv2: Failed to get template IPC client result: Message contains null capability pointer.
12025-12-31T16:42:09.308014Z ERROR bitcoin_core_sv2: Failed to create new template IPC client: CapnpError(Error { kind: MessageContainsNullCapabilityPointer, extra: "" })

this makes me conclude that Bitcoin Core will not accept a CreateNewBlock with set_block_reserved_weight < 2000, which feels aligned with the rationale behind https://github.com/stratum-mining/sv2-tp/pull/79

the test was saved on branch 2025-12-31-test-bitcoin-core-33965 of my fork, with the following commits:

7b343fba4ad506dba179b1f5fbcb99598617dafe which disables clamping 4 * CoinbaseOutputConstraints.coinbase_output_max_additional_size to 2000
951d5d14456fa861eb6b765cd65f05a94b3c3dfa which adds the modifications to integration_tests_sv2 code needed for this test (originally commented out and hardcoded with a path from my setup, but can be easily enabled/modified)
5cb733fc169fcd39293222fb1e600f543308a39d which adds the test_33965 Integration Test

the integration test can be executed with:

0git clone https://github.com/plebhash/sv2-apps -b 2025-12-31-test-bitcoin-core-33965
1cd sv2-apps/integration-tests
2cargo t test_33965 -- --nocapture

(I actually recommend cargo nextest run test_33965 --nocapture because it makes it easier to kill the tests without leaving zombie processes behind, but it’s optional)

Sjors commented at 3:21 am on January 1, 2026: member

@plebhash thanks for testing! So it looks like this PR works.

this makes me conclude that Bitcoin Core will not accept a CreateNewBlock with set_block_reserved_weight < 2000, which feels aligned with the rationale behind stratum-mining/sv2-tp#79

Good to know that it’s not quietly increasing the value, but rather just fails. Even though the error could be better.

in src/node/types.h:44 in 31325c1782

40@@ -41,7 +41,7 @@ struct BlockCreateOptions {
41     bool use_mempool{true};
42     /**
43      * The default reserved weight for the fixed-size block header,
44-     * transaction count and coinbase transaction.
45+     * transaction count and coinbase transaction. Minimum: 2000 weight units.

ryanofsky commented at 7:40 pm on January 7, 2026:

In commit “mining: fix -blockreservedweight shadows IPC option” (336826ad0e70978783dcc0777ad1f01d3dbc1698)

Maybe update the comment to say “2000 weight units (MINIMUM_BLOCK_RESERVED_WEIGHT)”. This would be helpful when grepping the constant and seeing how it is used & what may need to be updated if it changes.

in src/node/interfaces.cpp:969 in 31325c1782

962@@ -963,6 +963,11 @@ class MinerImpl : public Mining
963 
964     std::unique_ptr<BlockTemplate> createNewBlock(const BlockCreateOptions& options) override
965     {
966+        // Enforce minimum block_reserved_weight to avoid silently clamping it
967+        if (options.block_reserved_weight && options.block_reserved_weight < MINIMUM_BLOCK_RESERVED_WEIGHT) {
968+            return {};
969+        }

ryanofsky commented at 7:53 pm on January 7, 2026:

In commit “mining: fix -blockreservedweight shadows IPC option” (336826ad0e70978783dcc0777ad1f01d3dbc1698)

It would seem better to raise an exception with an error message here instead of returning null, since this probably indicates a programming error, and it would be helpful to callers to know what specifically is wrong. (Also, this method is only currently documented to return null on shutdown.)

Separately, it would also would be good to enforce this limit in the mining code instead of the interfaces code, so all the limits are enforced in one place. But this could be something for the followup #33966

Sjors commented at 3:10 am on January 8, 2026:

Added the exception. Followup is implemented in the other PR.

in test/functional/interface_ipc.py:235 in 336826ad0e

230@@ -222,6 +231,8 @@ async def async_routine():
231                 template4 = await wait_and_do(
232                     wait_next_template(template2, stack, ctx, waitoptions),
233                     lambda: miniwallet.send_self_transfer(fee_rate=10, from_node=self.nodes[0]))
234+                # Before bitcoin/bitcoin#33965 this would timeout due to -blockreservedweight
235+                # shadowing our waitoptions.blockReservedWeight.

ryanofsky commented at 8:05 pm on January 7, 2026:

In commit “mining: fix -blockreservedweight shadows IPC option” (336826ad0e70978783dcc0777ad1f01d3dbc1698)

Would suggest deleting this comment because it’s confusing to reference past behavior here. Instead I think it would be better to expand the “Set an absurd reserved weight” comment above. It can say the test confirms that the -blockreservedweight value is ignored by sending a transaction that would not fit with in a template with a huge reserved weight, and confirming the transaction does appear.

Sjors commented at 3:10 am on January 8, 2026:

Done, and expanded the comment at the top.

in src/node/interfaces.cpp:966 in 31325c1782

962@@ -963,6 +963,11 @@ class MinerImpl : public Mining
963 
964     std::unique_ptr<BlockTemplate> createNewBlock(const BlockCreateOptions& options) override
965     {
966+        // Enforce minimum block_reserved_weight to avoid silently clamping it

ryanofsky commented at 8:16 pm on January 7, 2026:

In commit “mining: enforce minimum reserved weight for IPC” (31325c1782e542601b3de8bd8bafe0af372830e9)

Could comment mention why this wants to reject a lower value instead of clamping it? I saw previous discussion https://github.com/stratum-mining/sv2-tp/pull/51 but couldn’t really follow it.

I would agree in general explicitly enforcing a limit is better than silently changing a value, but was wondering if there’s some more specific problem which led to this change?

Sjors commented at 2:33 am on January 8, 2026:

I initially thought that block_reserved_weight wasn’t clamped via IPC, and that there was no minimum enforced. But now I think it’s better to just enforce the same limit as we do for -blockreservedweight (and do so through an error, not clamping).

I guess the specific problem was confusion over the different behaviours.

ryanofsky approved

ryanofsky commented at 8:28 pm on January 7, 2026: contributor

Code review ACK 31325c1782e542601b3de8bd8bafe0af372830e9

Many changes since last review.

Main code change in second commit is a straightforward fix using std::optional to prevent ApplyArgsManOptions from overriding the block_reserved_weight value provided by IPC and ignoring it.

The test improvements in the first commit are also nice, and the change in the third commit to enforce the MINIMUM_BLOCK_RESERVED_WEIGHT instead of silently clamping it seem good.

I left some suggestions below but none are important.

Sjors force-pushed on Jan 8, 2026

test: have {create_block,wait_next}_template return None

Refactor the create_block_template and wait_next_template helpers in
interface_ipc.py to return None if they time out or fail. It makes the
test easier to read and provides a more clear error message in case of
a regression.

74709c0f89

mining: fix -blockreservedweight shadows IPC option

The -blockreservedweight startup option should only affect RPC code,
because IPC clients (currently) do not have a way to signal their intent
to use the node default (the BlockCreateOptions struct defaults
merely document a recommendation for client software).

Before this commit however, if the user set -blockreservedweight
then ApplyArgsManOptions would cause the block_reserved_weight
option passed by IPC clients to be ignored. Users who don't set
this value were not affected.

Fix this by making BlockCreateOptions::block_reserved_weight an
std::optional.

Internal interface users, such as the RPC call sites, don't set a
value so -blockreservedweight is used. Whereas IPC clients do set
a value which is no longer ignored.

Test coverage is added.

mining_basic.py already ensured -blockreservedweight is enforced by
mining RPC methods. This commit adds coverage for Mining interface IPC
clients. It also verifies that -blockreservedweight has no effect on
them.

Co-Authored-By: Russell Yanofsky <russ@yanofsky.org>

b9e18ae532

Sjors force-pushed on Jan 8, 2026

Sjors commented at 3:19 am on January 8, 2026: member

Addressed comments and also rebased (needed for #33966).

Even though the error could be better. @plebhash it should throw a more useful error now

DrahtBot added the label CI failed on Jan 8, 2026

DrahtBot removed the label CI failed on Jan 8, 2026

in test/functional/interface_ipc.py:281 in d3db528598

276+                self.log.debug("Enforce minimum reserved weight for IPC clients too")
277+                opts.blockReservedWeight = 0
278+                try:
279+                    await mining.createNewBlock(opts)
280+                    raise AssertionError("createNewBlock unexpectedly succeeded")
281+                except Exception as e:

ryanofsky commented at 12:19 pm on January 8, 2026:

In commit “mining: enforce minimum reserved weight for IPC” (d3db5285989f7b3fef7436a37ce928065be2804e)

Might be nice to make exception more specific to expected error like:

 0--- a/test/functional/interface_ipc.py
 1+++ b/test/functional/interface_ipc.py
 2@@ -278,8 +278,9 @@ class IPCInterfaceTest(BitcoinTestFramework):
 3                 try:
 4                     await mining.createNewBlock(opts)
 5                     raise AssertionError("createNewBlock unexpectedly succeeded")
 6-                except Exception as e:
 7-                    assert "must be at least" in str(e)
 8+                except capnp.lib.capnp.KjException as e:
 9+                    assert_equal(e.description, "remote exception: std::exception: block_reserved_weight (0) must be at least 2000 weight units")
10+                    assert_equal(e.type, "FAILED")
11 
12                 # Restore opts
13                 opts.blockReservedWeight = 4000

Sjors commented at 2:04 am on January 9, 2026:

Done

ryanofsky approved

ryanofsky commented at 12:24 pm on January 8, 2026: contributor

Code review ACK d3db5285989f7b3fef7436a37ce928065be2804e just making reserved weight check into an exception and improving comments since last review. Thanks for the updates!

mining: enforce minimum reserved weight for IPC

Previously a lower value was silently clamped to MINIMUM_BLOCK_RESERVED_WEIGHT.

d443301666

Sjors force-pushed on Jan 9, 2026

mining: fix -blockreservedweight shadows IPC option #33965

Code Coverage & Benchmarks

Reviews

Conflicts