-
Notifications
You must be signed in to change notification settings - Fork 38.7k
mining: fix -blockreservedweight shadows IPC option #33965
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code Coverage & BenchmarksFor details see: https://corecheck.dev/bitcoin/bitcoin/pulls/33965. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please copy-paste ConflictsReviewers, this pull request conflicts with the following ones:
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first. |
b24fb7b to
46d901c
Compare
|
🚧 At least one of the CI tasks failed. HintsTry to run the tests locally, according to the documentation. However, a CI failure may still
Leave a comment here, if you need help tracking down a confusing failure. |
|
Concept ACK. Would suggest updating 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 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. |
8e99655 to
4679a95
Compare
sv2-tp always sets the value (based on 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 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:
I added documentation to make this behavior clear. |
4679a95 to
1f2f4ab
Compare
ryanofsky
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code review ACK 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.
1f2f4ab to
4d68614
Compare
ryanofsky
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code review ACK 4d686142e079765cd31851481deb70659a9e9376. Just updated a comment since last review. Still looks good! Only minor comments below, not important
4d68614 to
54d3054
Compare
ryanofsky
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code review ACK 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.
54d3054 to
6d7c3ee
Compare
|
Rebased after #34003. I added an initial test refactor commit to have the 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. |
src/node/interfaces.cpp
Outdated
|
|
||
| std::unique_ptr<BlockTemplate> createNewBlock(const BlockCreateOptions& options) override | ||
| { | ||
| // Enfore minimum block_reserved_weight to avoid silently clamping it |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
6d7c3ee to
31325c1
Compare
|
@plebhash maybe you can you give this a quick test against the SRI integration? Without this PR, if you set |
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 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) |
|
@plebhash in the functional test for this PR I simply set |
|
ok I wrote an Integration Test with the following methodology:
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 note: Pool originally generates but internally, its here are the results: WITH clamping
WITHOUT clamping
both errors for this makes me conclude that Bitcoin Core will not accept a the test was saved on branch
the integration test can be executed with: (I actually recommend |
|
@plebhash thanks for testing! So it looks like this PR works.
Good to know that it's not quietly increasing the value, but rather just fails. Even though the error could be better. |
ryanofsky
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code review ACK 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.
31325c1 to
264fdbb
Compare
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]>
264fdbb to
d3db528
Compare
ryanofsky
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code review ACK 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.
d3db528 to
d443301
Compare
ryanofsky
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code review ACK d443301, just catching more specific python exception in test since last review
|
🐙 This pull request conflicts with the target branch and needs rebase. |

Also enforce
MINIMUM_BLOCK_RESERVED_WEIGHTfor IPC clients.The
-blockreservedweightstartup option should only affect RPC code, because IPC clients (currently) do not have a way to signal their intent to use the node default (theBlockCreateOptionsstruct defaults merely document a recommendation for client software).Before this PR however, if the user set
-blockreservedweightthenApplyArgsManOptionswould cause theblock_reserved_weightoption 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_templateandwait_next_templatehelpers.mining_basic.pyalready ensured-blockreservedweightis enforced by mining RPC methods. The second commit adds coverage for Mining interface IPC clients. It also verifies that-blockreservedweighthas no effect on them.The third commit enforces
MINIMUM_BLOCK_RESERVED_WEIGHTfor IPC clients. Previously lower values were quietly clamped.