Skip to content

Conversation

@Sjors
Copy link
Member

@Sjors Sjors commented Nov 28, 2025

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
Copy link
Contributor

DrahtBot commented Nov 28, 2025

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

@DrahtBot
Copy link
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.

Hints

Try to run the tests locally, according to the documentation. However, a CI failure may still
happen due to a number of reasons, for example:

  • Possibly due to a silent merge conflict (the changes in this pull request being
    incompatible with the current code in the target branch). If so, make sure to rebase on the latest
    commit of the target branch.

  • A sanitizer issue, which can only be found by compiling with the sanitizer and running the
    affected test.

  • An intermittent issue.

Leave a comment here, if you need help tracking down a confusing failure.

@ryanofsky
Copy link
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:

diff

--- a/src/node/miner.cpp
+++ b/src/node/miner.cpp
@@ -78,11 +78,11 @@ void RegenerateCommitments(CBlock& block, ChainstateManager& chainman)
 
 static BlockAssembler::Options ClampOptions(BlockAssembler::Options options)
 {
-    options.block_reserved_weight = std::clamp<size_t>(options.block_reserved_weight, MINIMUM_BLOCK_RESERVED_WEIGHT, MAX_BLOCK_WEIGHT);
+    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);
     options.coinbase_output_max_additional_sigops = std::clamp<size_t>(options.coinbase_output_max_additional_sigops, 0, MAX_BLOCK_SIGOPS_COST);
     // Limit weight to between block_reserved_weight and MAX_BLOCK_WEIGHT for sanity:
     // block_reserved_weight can safely exceed -blockmaxweight, but the rest of the block template will be empty.
-    options.nBlockMaxWeight = std::clamp<size_t>(options.nBlockMaxWeight, options.block_reserved_weight, MAX_BLOCK_WEIGHT);
+    options.nBlockMaxWeight = std::clamp<size_t>(options.nBlockMaxWeight, *options.block_reserved_weight, MAX_BLOCK_WEIGHT);
     return options;
 }
 
@@ -102,13 +102,15 @@ void ApplyArgsManOptions(const ArgsManager& args, BlockAssembler::Options& optio
         if (const auto parsed{ParseMoney(*blockmintxfee)}) options.blockMinFeeRate = CFeeRate{*parsed};
     }
     options.print_modified_fee = args.GetBoolArg("-printpriority", options.print_modified_fee);
-    options.block_reserved_weight = args.GetIntArg("-blockreservedweight", options.block_reserved_weight);
+    if (!options.block_reserved_weight) {
+        options.block_reserved_weight = args.GetIntArg("-blockreservedweight");
+    }
 }
 
 void BlockAssembler::resetBlock()
 {
     // Reserve space for fixed-size block header, txs count, and coinbase tx.
-    nBlockWeight = m_options.block_reserved_weight;
+    nBlockWeight = *m_options.block_reserved_weight;
     nBlockSigOpsCost = m_options.coinbase_output_max_additional_sigops;
 
     // These counters do not include coinbase tx
--- a/src/node/types.h
+++ b/src/node/types.h
@@ -42,7 +42,7 @@ struct BlockCreateOptions {
      * The default reserved weight for the fixed-size block header,
      * transaction count and coinbase transaction.
      */
-    size_t block_reserved_weight{DEFAULT_BLOCK_RESERVED_WEIGHT};
+    std::optional<size_t> block_reserved_weight{};
     /**
      * The maximum additional sigops which the pool will add in coinbase
      * transaction outputs.

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

diff

--- a/src/node/interfaces.cpp
+++ b/src/node/interfaces.cpp
@@ -963,8 +963,12 @@ public:
         // Ensure m_tip_block is set so consumers of BlockTemplate can rely on that.
         if (!waitTipChanged(uint256::ZERO, MillisecondsDouble::max())) return {};
 
+        // Construct block assembler options from block create options and
+        // ArgsManager preferences. Intentionally do not call
+        // ReadBlockCreateArgs, so IPC create options will take precedence over
+        // ArgsManager values.
         BlockAssembler::Options assemble_options{options};
-        ApplyArgsManOptions(*Assert(m_node.args), assemble_options);
+        ReadBlockAssemblerArgs(*Assert(m_node.args), assemble_options);
         return std::make_unique<BlockTemplateImpl>(assemble_options, BlockAssembler{chainman().ActiveChainstate(), context()->mempool.get(), assemble_options}.CreateNewBlock(), m_node);
     }
 
--- a/src/node/miner.cpp
+++ b/src/node/miner.cpp
@@ -94,7 +94,7 @@ BlockAssembler::BlockAssembler(Chainstate& chainstate, const CTxMemPool* mempool
 {
 }
 
-void ApplyArgsManOptions(const ArgsManager& args, BlockAssembler::Options& options)
+void ReadBlockAssemblerArgs(const ArgsManager& args, BlockAssembler::Options& options)
 {
     // Block resource limits
     options.nBlockMaxWeight = args.GetIntArg("-blockmaxweight", options.nBlockMaxWeight);
@@ -102,6 +102,10 @@ void ApplyArgsManOptions(const ArgsManager& args, BlockAssembler::Options& optio
         if (const auto parsed{ParseMoney(*blockmintxfee)}) options.blockMinFeeRate = CFeeRate{*parsed};
     }
     options.print_modified_fee = args.GetBoolArg("-printpriority", options.print_modified_fee);
+}
+
+void ReadBlockCreateArgs(const ArgsManager& args, BlockCreateOptions& options)
+{
     options.block_reserved_weight = args.GetIntArg("-blockreservedweight", options.block_reserved_weight);
 }
 
--- a/src/node/miner.h
+++ b/src/node/miner.h
@@ -131,8 +131,11 @@ int64_t UpdateTime(CBlockHeader* pblock, const Consensus::Params& consensusParam
 /** Update an old GenerateCoinbaseCommitment from CreateNewBlock after the block txs have changed */
 void RegenerateCommitments(CBlock& block, ChainstateManager& chainman);
 
+/** Apply -blockreservedweight options from ArgsManager to BlockCreateOptions. */
+void ReadBlockCreateArgs(const ArgsManager& args, BlockCreateOptions& options);
+
 /** Apply -blockmintxfee and -blockmaxweight options from ArgsManager to BlockAssembler options. */
-void ApplyArgsManOptions(const ArgsManager& gArgs, BlockAssembler::Options& options);
+void ReadBlockAssemblerArgs(const ArgsManager& args, BlockAssembler::Options& options);
 
 /* Compute the block's merkle root, insert or replace the coinbase transaction and the merkle root into the block */
 void AddMerkleRootAndCoinbase(CBlock& block, CTransactionRef coinbase, uint32_t version, uint32_t timestamp, uint32_t nonce);
--- a/src/rpc/mining.cpp
+++ b/src/rpc/mining.cpp
@@ -161,11 +161,13 @@ static bool GenerateBlock(ChainstateManager& chainman, CBlock&& block, uint64_t&
     return true;
 }
 
-static UniValue generateBlocks(ChainstateManager& chainman, Mining& miner, const CScript& coinbase_output_script, int nGenerate, uint64_t nMaxTries)
+static UniValue generateBlocks(ChainstateManager& chainman, ArgsManager& args, Mining& miner, const CScript& coinbase_output_script, int nGenerate, uint64_t nMaxTries)
 {
     UniValue blockHashes(UniValue::VARR);
+    node::BlockCreateOptions options{ .coinbase_output_script = coinbase_output_script };
+    ReadBlockCreateArgs(args, options);
     while (nGenerate > 0 && !chainman.m_interrupt) {
-        std::unique_ptr<BlockTemplate> block_template(miner.createNewBlock({ .coinbase_output_script = coinbase_output_script }));
+        std::unique_ptr<BlockTemplate> block_template(miner.createNewBlock(options));
         CHECK_NONFATAL(block_template);
 
         std::shared_ptr<const CBlock> block_out;
@@ -247,9 +249,10 @@ static RPCHelpMan generatetodescriptor()
 
     NodeContext& node = EnsureAnyNodeContext(request.context);
     Mining& miner = EnsureMining(node);
+    ArgsManager& args = EnsureArgsman(node);
     ChainstateManager& chainman = EnsureChainman(node);
 
-    return generateBlocks(chainman, miner, coinbase_output_script, num_blocks, max_tries);
+    return generateBlocks(chainman, args, miner, coinbase_output_script, num_blocks, max_tries);
 },
     };
 }
@@ -293,11 +296,12 @@ static RPCHelpMan generatetoaddress()
 
     NodeContext& node = EnsureAnyNodeContext(request.context);
     Mining& miner = EnsureMining(node);
+    ArgsManager& args = EnsureArgsman(node);
     ChainstateManager& chainman = EnsureChainman(node);
 
     CScript coinbase_output_script = GetScriptForDestination(destination);
 
-    return generateBlocks(chainman, miner, coinbase_output_script, num_blocks, max_tries);
+    return generateBlocks(chainman, args, miner, coinbase_output_script, num_blocks, max_tries);
 },
     };
 }
@@ -345,6 +349,7 @@ static RPCHelpMan generateblock()
 
     NodeContext& node = EnsureAnyNodeContext(request.context);
     Mining& miner = EnsureMining(node);
+    ArgsManager& args = EnsureArgsman(node);
     const CTxMemPool& mempool = EnsureMemPool(node);
 
     std::vector<CTransactionRef> txs;
@@ -374,9 +379,11 @@ static RPCHelpMan generateblock()
 
     ChainstateManager& chainman = EnsureChainman(node);
     {
+        node::BlockCreateOptions options{.use_mempool = false, .coinbase_output_script = coinbase_output_script};
+        ReadBlockCreateArgs(args, options);
         LOCK(chainman.GetMutex());
         {
-            std::unique_ptr<BlockTemplate> block_template{miner.createNewBlock({.use_mempool = false, .coinbase_output_script = coinbase_output_script})};
+            std::unique_ptr<BlockTemplate> block_template{miner.createNewBlock(options)};
             CHECK_NONFATAL(block_template);
 
             block = block_template->getBlock();
@@ -471,7 +478,8 @@ static RPCHelpMan getmininginfo()
     obj.pushKV("networkhashps",    getnetworkhashps().HandleRequest(request));
     obj.pushKV("pooledtx",         (uint64_t)mempool.size());
     BlockAssembler::Options assembler_options;
-    ApplyArgsManOptions(*node.args, assembler_options);
+    ReadBlockCreateArgs(*node.args, assembler_options);
+    ReadBlockAssemblerArgs(*node.args, assembler_options);
     obj.pushKV("blockmintxfee", ValueFromAmount(assembler_options.blockMinFeeRate.GetFeePerK()));
     obj.pushKV("chain", chainman.GetParams().GetChainTypeString());
 
@@ -871,7 +879,10 @@ static RPCHelpMan getblocktemplate()
         time_start = GetTime();
 
         // Create new block
-        block_template = miner.createNewBlock();
+        ArgsManager& args = EnsureArgsman(node);
+        node::BlockCreateOptions options;
+        ReadBlockCreateArgs(args, options);
+        block_template = miner.createNewBlock(options);
         CHECK_NONFATAL(block_template);
 
 
--- a/src/test/util/mining.cpp
+++ b/src/test/util/mining.cpp
@@ -138,6 +138,7 @@ std::shared_ptr<CBlock> PrepareBlock(const NodeContext& node, const CScript& coi
 {
     BlockAssembler::Options assembler_options;
     assembler_options.coinbase_output_script = coinbase_scriptPubKey;
-    ApplyArgsManOptions(*node.args, assembler_options);
+    ReadBlockCreateArgs(*node.args, assembler_options);
+    ReadBlockAssemblerArgs(*node.args, assembler_options);
     return PrepareBlock(node, assembler_options);
 }

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

@Sjors Sjors force-pushed the 2025/11/ipc-reserve branch 2 times, most recently from 8e99655 to 4679a95 Compare December 2, 2025 10:41
@Sjors
Copy link
Member Author

Sjors commented Dec 2, 2025

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.:

blockReservedWeight @1 :UInt64 $Proxy.name("block_reserved_weight");
hasBlockReservedWeight @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.

@Sjors
Copy link
Member Author

Sjors commented Dec 2, 2025

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

@DrahtBot DrahtBot removed the CI failed label Dec 2, 2025
Copy link
Contributor

@ryanofsky ryanofsky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

blockReservedWeight : union { none @1 :Void; value @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 Sjors force-pushed the 2025/11/ipc-reserve branch from 1f2f4ab to 4d68614 Compare December 2, 2025 18:48
Copy link
Contributor

@ryanofsky ryanofsky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

@ryanofsky ryanofsky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code review ACK 54d3054. 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.

@Sjors
Copy link
Member Author

Sjors commented Dec 19, 2025

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 stratum-mining/sv2-tp#51 (comment). I could make that it's own PR but it's rather trivial.


std::unique_ptr<BlockTemplate> createNewBlock(const BlockCreateOptions& options) override
{
// Enfore minimum block_reserved_weight to avoid silently clamping it
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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]

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How many LLM's does it take to fix a typo? :-)
Scherm­afbeelding 2025-12-19 om 16 59 50

@Sjors
Copy link
Member Author

Sjors commented Dec 30, 2025

@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
Copy link

plebhash commented Dec 30, 2025

@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
Copy link
Member Author

Sjors commented Dec 31, 2025

@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
Copy link

plebhash commented Dec 31, 2025

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:

2025-12-31T16:42:09.307986Z ERROR bitcoin_core_sv2: Failed to get template IPC client result: Message contains null capability pointer.
2025-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 stratum-mining/sv2-tp#79


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

the integration test can be executed with:

git clone https://github.com/plebhash/sv2-apps -b 2025-12-31-test-bitcoin-core-33965
cd sv2-apps/integration-tests
cargo 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
Copy link
Member Author

Sjors commented Jan 1, 2026

@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.

Copy link
Contributor

@ryanofsky ryanofsky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code review ACK 31325c1

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 Sjors force-pushed the 2025/11/ipc-reserve branch from 31325c1 to 264fdbb Compare January 8, 2026 03:10
Sjors and others added 2 commits January 8, 2026 10:18
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.
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 <[email protected]>
@Sjors Sjors force-pushed the 2025/11/ipc-reserve branch from 264fdbb to d3db528 Compare January 8, 2026 03:18
@Sjors
Copy link
Member Author

Sjors commented Jan 8, 2026

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

Even though the error could be better.

@plebhash it should throw a more useful error now

Copy link
Contributor

@ryanofsky ryanofsky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Previously a lower value was silently clamped to MINIMUM_BLOCK_RESERVED_WEIGHT.
@Sjors Sjors force-pushed the 2025/11/ipc-reserve branch from d3db528 to d443301 Compare January 9, 2026 02:03
Copy link
Contributor

@ryanofsky ryanofsky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code review ACK d443301, just catching more specific python exception in test since last review

@DrahtBot
Copy link
Contributor

🐙 This pull request conflicts with the target branch and needs rebase.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants