-
Notifications
You must be signed in to change notification settings - Fork 39.1k
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -67,6 +67,7 @@ | |
| #include <any> | ||
| #include <memory> | ||
| #include <optional> | ||
| #include <stdexcept> | ||
| #include <utility> | ||
|
|
||
| #include <boost/signals2/signal.hpp> | ||
|
|
@@ -969,6 +970,16 @@ class MinerImpl : public Mining | |
|
|
||
| std::unique_ptr<BlockTemplate> createNewBlock(const BlockCreateOptions& options) override | ||
| { | ||
| // Reject too-small values instead of clamping so callers don't silently | ||
| // end up mining with different options than requested. This matches the | ||
| // behavior of the `-blockreservedweight` startup option, which rejects | ||
| // values below MINIMUM_BLOCK_RESERVED_WEIGHT. | ||
| if (options.block_reserved_weight && options.block_reserved_weight < MINIMUM_BLOCK_RESERVED_WEIGHT) { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In "mining: enforce minimum reserved weight for IPC" 8bc1cb7 The interface implementation has a goal of not having implementation code in here, since we are doing this, perhaps it's time to have a wrapper for all this. which should include 8bc1cb7#r2749269557 suggestion and any other future additions?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. re: #33965 (comment)
This should be happening in followup #33966 which unifies the startup & ipc checks.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nice |
||
| throw std::runtime_error(strprintf("block_reserved_weight (%zu) must be at least %u weight units", | ||
| *options.block_reserved_weight, | ||
| MINIMUM_BLOCK_RESERVED_WEIGHT)); | ||
| } | ||
|
|
||
| // Ensure m_tip_block is set so consumers of BlockTemplate can rely on that. | ||
| if (!waitTipChanged(uint256::ZERO, MillisecondsDouble::max())) return {}; | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -78,11 +78,12 @@ 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); | ||
| // Apply DEFAULT_BLOCK_RESERVED_WEIGHT when the caller left it unset. | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In "mining: fix -blockreservedweight shadows IPC option" f69320d Why make the distinction between RPC and IPC clients? The narrative of this commit should rather be future proof and changed to: it can be overridded by runtime via the IPC or RPC.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. re: #33965 (comment)
The commit description is just describing what the commit is changing, which is only IPC clients. The IPC and RPC interfaces are different, with IPC clients currently required to specify RPC behavior is unchanged by this commit. IPC behavior is just being changed so the IPC value is applied instead of ignored when the |
||
| 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 +103,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 = *Assert(m_options.block_reserved_weight); | ||
| nBlockSigOpsCost = m_options.coinbase_output_max_additional_sigops; | ||
|
|
||
| // These counters do not include coinbase tx | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -8,6 +8,7 @@ | |
| from io import BytesIO | ||
| from test_framework.blocktools import NULL_OUTPOINT | ||
| from test_framework.messages import ( | ||
| MAX_BLOCK_WEIGHT, | ||
| CTransaction, | ||
| CTxIn, | ||
| CTxOut, | ||
|
|
@@ -160,6 +161,7 @@ async def async_routine(): | |
| async with AsyncExitStack() as stack: | ||
| self.log.debug("Create a template") | ||
| template = await mining_create_block_template(mining, stack, ctx, self.default_block_create_options) | ||
| assert template is not None | ||
|
|
||
| self.log.debug("Test some inspectors of Template") | ||
| header = (await template.getBlockHeader(ctx)).result | ||
|
|
@@ -179,23 +181,26 @@ async def async_routine(): | |
| template2 = await wait_and_do( | ||
| mining_wait_next_template(template, stack, ctx, waitoptions), | ||
| lambda: self.generate(self.nodes[0], 1)) | ||
| assert template2 is not None | ||
| block2 = await mining_get_block(template2, ctx) | ||
| assert_equal(len(block2.vtx), 1) | ||
|
|
||
| self.log.debug("Wait for another, but time out") | ||
| template3 = await template2.waitNext(ctx, waitoptions) | ||
| assert_equal(template3._has("result"), False) | ||
| template3 = await mining_wait_next_template(template2, stack, ctx, waitoptions) | ||
| assert template3 is None | ||
|
|
||
| self.log.debug("Wait for another, get one after increase in fees in the mempool") | ||
| template4 = await wait_and_do( | ||
| mining_wait_next_template(template2, stack, ctx, waitoptions), | ||
| lambda: self.miniwallet.send_self_transfer(fee_rate=10, from_node=self.nodes[0])) | ||
| assert template4 is not None | ||
| block3 = await mining_get_block(template4, ctx) | ||
| assert_equal(len(block3.vtx), 2) | ||
|
|
||
| self.log.debug("Wait again, this should return the same template, since the fee threshold is zero") | ||
| waitoptions.feeThreshold = 0 | ||
| template5 = await mining_wait_next_template(template4, stack, ctx, waitoptions) | ||
| assert template5 is not None | ||
| block4 = await mining_get_block(template5, ctx) | ||
| assert_equal(len(block4.vtx), 2) | ||
| waitoptions.feeThreshold = 1 | ||
|
|
@@ -204,24 +209,65 @@ async def async_routine(): | |
| template6 = await wait_and_do( | ||
| mining_wait_next_template(template5, stack, ctx, waitoptions), | ||
| lambda: self.miniwallet.send_self_transfer(fee_rate=10, from_node=self.nodes[0])) | ||
| assert template6 is not None | ||
| block4 = await mining_get_block(template6, ctx) | ||
| assert_equal(len(block4.vtx), 3) | ||
|
|
||
| self.log.debug("Wait for another, but time out, since the fee threshold is set now") | ||
| template7 = await template6.waitNext(ctx, waitoptions) | ||
| assert_equal(template7._has("result"), False) | ||
| template7 = await mining_wait_next_template(template6, stack, ctx, waitoptions) | ||
| assert template7 is None | ||
|
|
||
| self.log.debug("interruptWait should abort the current wait") | ||
| async def wait_for_block(): | ||
| new_waitoptions = self.capnp_modules['mining'].BlockWaitOptions() | ||
| new_waitoptions.timeout = timeout * 60 # 1 minute wait | ||
| new_waitoptions.feeThreshold = 1 | ||
| template7 = await template6.waitNext(ctx, new_waitoptions) | ||
| assert_equal(template7._has("result"), False) | ||
| template7 = await mining_wait_next_template(template6, stack, ctx, new_waitoptions) | ||
| assert template7 is None | ||
| await wait_and_do(wait_for_block(), template6.interruptWait()) | ||
|
|
||
| asyncio.run(capnp.run(async_routine())) | ||
|
|
||
| def run_ipc_option_override_test(self): | ||
| self.log.info("Running IPC option override test") | ||
| # Set an absurd reserved weight. `-blockreservedweight` is RPC-only, so | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In commit "mining: fix -blockreservedweight shadows IPC option" (d3e4952) I feel like this description would be easier to understand if it began with a sentence saying what this is trying to check like "Confirm that
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks, will improve if I need to retouch.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Taken in #33966. |
||
| # with this setting RPC templates would be empty. IPC clients set | ||
| # blockReservedWeight per template request and are unaffected; later in | ||
| # the test the IPC template includes a mempool transaction. | ||
| self.restart_node(0, extra_args=[f"-blockreservedweight={MAX_BLOCK_WEIGHT}"]) | ||
|
|
||
| async def async_routine(): | ||
| ctx, mining = await self.make_mining_ctx() | ||
| self.miniwallet.send_self_transfer(fee_rate=10, from_node=self.nodes[0]) | ||
|
|
||
| async with AsyncExitStack() as stack: | ||
| opts = self.capnp_modules['mining'].BlockCreateOptions() | ||
| opts.useMempool = True | ||
| opts.blockReservedWeight = 4000 | ||
| opts.coinbaseOutputMaxAdditionalSigops = 0 | ||
| template = await mining_create_block_template(mining, stack, ctx, opts) | ||
| assert template is not None | ||
| block = await mining_get_block(template, ctx) | ||
| assert_equal(len(block.vtx), 2) | ||
|
|
||
| self.log.debug("Use absurdly large reserved weight to force an empty template") | ||
| opts.blockReservedWeight = MAX_BLOCK_WEIGHT | ||
| empty_template = await mining_create_block_template(mining, stack, ctx, opts) | ||
| assert empty_template is not None | ||
| empty_block = await mining_get_block(empty_template, ctx) | ||
| assert_equal(len(empty_block.vtx), 1) | ||
|
|
||
| self.log.debug("Enforce minimum reserved weight for IPC clients too") | ||
| opts.blockReservedWeight = 0 | ||
| try: | ||
| await mining.createNewBlock(opts) | ||
| raise AssertionError("createNewBlock unexpectedly succeeded") | ||
| except capnp.lib.capnp.KjException as e: | ||
| assert_equal(e.description, "remote exception: std::exception: block_reserved_weight (0) must be at least 2000 weight units") | ||
| assert_equal(e.type, "FAILED") | ||
|
|
||
| asyncio.run(capnp.run(async_routine())) | ||
|
|
||
| def run_coinbase_and_submission_test(self): | ||
| """Test coinbase construction (getCoinbaseTx, getCoinbaseCommitment) and block submission (submitSolution).""" | ||
| self.log.info("Running coinbase construction and submission test") | ||
|
|
@@ -304,6 +350,7 @@ def run_test(self): | |
| self.run_mining_interface_test() | ||
| self.run_block_template_test() | ||
| self.run_coinbase_and_submission_test() | ||
| self.run_ipc_option_override_test() | ||
|
|
||
|
|
||
| if __name__ == '__main__': | ||
|
|
||
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.
Just a question: Is there a good reason to only enforce the minimum here?
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.
Agreed, since the rationale is to match the startup option checks, it should be verified as well.
Uh oh!
There was an error while loading. Please reload this page.
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.
re: #33965 (comment)
Maybe this check could be expanded but followup #33966 should unify this with startup checks in init.cpp:
bitcoin/src/init.cpp
Lines 1071 to 1077 in 28d8607
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.
I'll look into this for the followup.
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.
Added maximum check in #33966.