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:

#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

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.

a62d946f88

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>

336826ad0e

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

mining: enforce minimum reserved weight for IPC

Previously a lower value was silently clamped to MINIMUM_BLOCK_RESERVED_WEIGHT.

31325c1782

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

mining: fix -blockreservedweight shadows IPC option #33965

Code Coverage & Benchmarks

Reviews

Conflicts