-
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 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
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>
- Loading branch information
There are no files selected for viewing
| 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, | ||
|
|
@@ -227,6 +228,37 @@ async def wait_for_block(): | |
|
|
||
| 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) | ||
|
|
||
| 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") | ||
|
|
@@ -309,6 +341,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.
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.
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)
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
block_reserved_weightand RPC clients not able to specify it at all.RPC behavior is unchanged by this commit. IPC behavior is just being changed so the IPC value is applied instead of ignored when the
-blockreservedweightconfig option is used.