-blockmaxweight startup option debug only
#32654
This PR updates -blockmaxweight startup option to be debug-only so that it will be hidden from help text.
The option is currently unlikely to be used on mainnet, after the addition of the new blockreservedweight option. however it can be useful for test and signet network see #32654 (comment)
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/32654.
See the guideline for information on the review process.
| Type | Reviewers |
|---|---|
| ACK | Sjors, fjahr, polespinasa |
| User requested bot ignore | ajtowns |
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
No conflicts as of last run.
1027@@ -1028,6 +1028,10 @@ bool AppInitParameterInteraction(const ArgsManager& args)
1028 }
1029 }
1030
1031+ if (args.IsArgSet("-blockmaxweight")) {
1032+ InitWarning(_("Option '-blockmaxweight' is set. This option is deprecated and will be removed in a future version."));
cACK
Needs release note
144@@ -145,8 +145,8 @@ def set_test_params(self):
145 self.noban_tx_relay = True
146 self.extra_args = [
147 [],
148- ["-blockmaxweight=68000"],
149- ["-blockmaxweight=32000"],
150+ ["-blockreservedweight=3932000"],
151+ ["-blockreservedweight=3968000"],
DEFAULT_BLOCK_RESERVED_WEIGHT (8000), I think.
336@@ -336,7 +337,7 @@ def test_block_max_weight(self):
337
338 self.log.info("Test -blockreservedweight startup option.")
339 # Lowering the -blockreservedweight by 4000 will allow for two more transactions.
340- self.restart_node(0, extra_args=[f"-datacarriersize={LARGE_VSIZE}", "-blockreservedweight=4000"])
341+ self.restart_node(0, extra_args=[f"-datacarriersize={LARGE_VSIZE}", "-blockreservedweight=4000"], expected_stderr=deprecation_warning)
blockmaxweight but the deprecation warning from init is still expected to show here. Looking at how this works in restart_node explains why: The warning is checked during stopping of the node and not during starting of node. I think it would be good to add at least some comment here how this works. Maybe there is also a more expressive way to handle it but I think a comment is good enough.
That’s right. It is checked during stopping of the node. Had to introduce that in https://github.com/bitcoin/bitcoin/commit/bf194c920cf768d1339d41aef1441a78e2f5fcbe#diff-4a04bc0b355c780033960e8c261ee9b6d3c452897e1dcd88a15d272512266c76
Didn’t find a better way to handle it though (at that moment). Probably we could just remove that stderr check and restart the node manually using first stop and then start. That could be clearer?
Concept NACK. Lowering the block max weight can be a useful thing to do to (eg, signet blocks are limited to 1M weight units in order to help generate fee pressure, and mildly discourage wasteful usage), and having to pretend to pad the coinbase is a very awkward alternative.
Not sure what you mean by “having to pretend to pad the coinbase is a very awkward alternative.”
The reservedweight isn’t just for the coinbase it also applies to the block header and transaction count. It seems reasonable to use the blockreservedweight option to lower the block weight to the desired magnitude.
Closed for now due to lack of conceptual support. cc @Sjors
The reservedweight isn’t just for the coinbase it also applies to the block header and transaction count. It seems reasonable to use the blockreservedweight option to lower the block weight to the desired magnitude.
Well, you can’t pad the block header or transaction count encoding? :)
I think it makes sense to combine the two values into one internal option that controls the maximum cumulative tx weight, but I think the options both individually make sense from a user’s point of view. (TBH, I think these options should move into the template request, rather than being node startup options)
I don’t think we should recommend -blockreservedweight as an alternative to -blockmaxweight, as that just adds confusion.
At this point -blockmaxweight seems useless in production, but it appears to be useful in test networks and our owns tests. We could hide the option under --help-debug in order to unclutter the --help output. But I wouldn’t drop it.
(TBH, I think these options should move into the template request, rather than being node startup options)
Yes, the BlockCreateOptions have block_reserved_weight variable. From recent @Sjors comment I think there’s no need for the blockmaxweight option for TP that’s why it was not added.
It makes sense to add these options to the template request RPC parameter as well, since test networks and other protocols still rely on the RPC feels natural for this to be runtime option.
My only concern with having both is the added complexity of two different ways to cap the block limit. Although unlikely, users could do something like what @pinheadmz mentioned #31384 (review) also see thread #31384 (review) this is the motivation for this PR.
We could hide the option under –help-debug in order to unclutter the –help output. But I wouldn’t drop it.
Yeah, I’m inclined to agree with this suggestion, since the primary use case is for test networks and our functional tests.
I can reopen and push this here if it sounds good to @ajtowns
I think it makes sense to combine the two values into one internal option that controls the maximum cumulative tx weight.
That sounds like a good idea. Could you expand on that?
(TBH, I think these options should move into the template request, rather than being node startup options)
Yes, the
BlockCreateOptionshaveblock_reserved_weightvariable. From recent @Sjors comment I think there’s no need for theblockmaxweightoption for TP that’s why it was not added.
I think it makes sense to combine the two values into one internal option that controls the maximum cumulative tx weight.
That sounds like a good idea. Could you expand on that?
We already combine them in ClampOptions; could do that ApplyArgsManOptions instead, and only have a single field for our internal structures. I guess something like:
0block_reserved_weight = clamp(block_reserved_weight, MAX_BLOCK_WEIGHT - nBlockMaxWeight, MAX_BLOCK_WEIGHT);
would allow replacing the nBlockWeight vs nMaxBlockWeight comparisons with nBlockWeight vs MAX_BLOCK_WEIGHT.
It makes sense to add these options to the template request RPC parameter as well, since test networks and other protocols still rely on the RPC feels natural for this to be runtime option.
My only concern with having both is the added complexity of two different ways to cap the block limit. Although unlikely, users could do something like what @pinheadmz mentioned #31384 (comment) also see thread #31384 (comment) this is the motivation for this PR.
I think clamping the values into reasonable ranges and just giving a best effort response to an impossible request is fine; checking and giving an error in response to an RPC request also seems fine.
We could hide the option under –help-debug in order to unclutter the –help output. But I wouldn’t drop it.
Yeah, I’m inclined to agree with this suggestion, since the primary use case is for test networks and our functional tests.
No particular objection to that.
No particular objection to that.
Done.
tACK e017ef3c7eb775e2cf999674df341be56f7ba72d
Although I don’t think this is super important, it’s nice to declutter the default --help output.
Meta point: it probably would have been better to open a fresh PR with such a scope change.
Apparently a thumbs down doesn’t make drahtbot remove my now inapplicable review? Maybe a comment will trigger an update? Anyway, I don’t have an opinion on whether this should be a debug option or not, so don’t intend to give an updated review. The diff looks correct for achieving the goal at least.
Being able to adjust the target block weight at getblocktemplate time without having to restart would have been helpful for clearing out the signet mempool the other day, fwiw.
Being able to adjust the target block weight at getblocktemplate time without having to restart would have been helpful for clearing out the signet mempool the other day, fwiw.
Apparently getblocktemplate is implemented in accordance with specifications of BIPs 22, 23, 9, and 145.
So I think this needs a new bip update for the new field I guess, I think @Sjors run into similar blocker while working on #27433?
If it worth having a bip update for the blockmaxweight field, then we can also add blockreservedweight as well.
ACK e017ef3c7eb775e2cf999674df341be56f7ba72d
Seems like moving this to debug is in line with what we do with test-only RPC.
tACK e017ef3c7eb775e2cf999674df341be56f7ba72d
Nothing to add to the comments already done.
643@@ -644,7 +644,7 @@ void SetupServerArgs(ArgsManager& argsman, bool can_listen_ipc)
644 argsman.AddArg("-whitelistrelay", strprintf("Add 'relay' permission to whitelisted peers with default permissions. This will accept relayed transactions even when not relaying transactions (default: %d)", DEFAULT_WHITELISTRELAY), ArgsManager::ALLOW_ANY, OptionsCategory::NODE_RELAY);
645
646
647- argsman.AddArg("-blockmaxweight=<n>", strprintf("Set maximum BIP141 block weight (default: %d)", DEFAULT_BLOCK_MAX_WEIGHT), ArgsManager::ALLOW_ANY, OptionsCategory::BLOCK_CREATION);
648+ argsman.AddArg("-blockmaxweight=<n>", strprintf("Set maximum BIP141 block weight (default: %d)", DEFAULT_BLOCK_MAX_WEIGHT), ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::BLOCK_CREATION);
Yeah, I will mention when the notes are ready to be edited.
Thanks