Skip to content

miner: clamp options instead of asserting #33222

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Aug 21, 2025
Merged

miner: clamp options instead of asserting #33222

merged 1 commit into from
Aug 21, 2025

Conversation

sipa
Copy link
Member

@sipa sipa commented Aug 19, 2025

The BlockAssembler::ClampOptions function currently doesn't actually clamp most of the provided settings, but asserts that some are in range. This made sense while it was a purely internal interface.

However, with the mining IPC interface exposed in #30510, these options are now externally accessible, and it is not entirely intuitive how to set them. In particular, calling Mining::createNewBlock with a default-constructed BlockCreateOptions will right now instantly crash the bitcoin node.

This isn't a security issue, as the IPC interface is considered trusted, but it is highly unexpected I think, and rather unergonomical to have the node crash while developing against the interface.

An alternative would be exposing a way for the interface to return a failure, but I think in this case, just correcting to reasonable values is acceptable.

@DrahtBot
Copy link
Contributor

DrahtBot commented Aug 19, 2025 •
edited
Loading

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

Reviews

See the guideline for information on the review process.

Type Reviewers
ACK stickies-v, ryanofsky, Sjors, achow101

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

@sipa
Copy link
Member Author

sipa commented Aug 19, 2025

This makes sense for v30.0, I think, if we're going to be shipping mining interface support (#31802 or variation thereof).

stickies-v
stickies-v approved these changes Aug 21, 2025
View reviewed changes
Copy link
Contributor

@stickies-v stickies-v left a comment

Choose a reason for hiding this comment

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

ACK 7392b8b

but I think in this case, just correcting to reasonable values is acceptable.

I agree. I generally support logging in situations like this, so that we at least give the user a chance to see something unexpected is happening without being unergonomic, but I think it's also fine as-is. We generally don't log for clamping, as far as I can tell.

@fanquake fanquake added this to the 30.0 milestone Aug 21, 2025
@fanquake fanquake requested review from Sjors and ryanofsky August 21, 2025 14:41
ryanofsky
ryanofsky approved these changes Aug 21, 2025
View reviewed changes
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 7392b8b. I think ideally this would throw an exception and return a clear error to the caller, or maybe log as stickies suggested, but clamping is much better than crashing.

@ryanofsky
Copy link
Contributor

ryanofsky commented Aug 21, 2025 •
edited
Loading

@Sjors maybe you have thoughts on this?

It would probably be good if we set default field values in the capnp file:

bitcoin/src/ipc/capnp/mining.capnp

Lines 40 to 41 in 8333aa5

blockReservedWeight @1 :UInt64 $Proxy.name("block_reserved_weight");
coinbaseOutputMaxAdditionalSigops @2 :UInt64 $Proxy.name("coinbase_output_max_additional_sigops");

that match the c++ defaults:

bitcoin/src/node/types.h

Lines 40 to 49 in 8333aa5

/**
* The default reserved weight for the fixed-size block header,
* transaction count and coinbase transaction.
*/
size_t block_reserved_weight{DEFAULT_BLOCK_RESERVED_WEIGHT};
/**
* The maximum additional sigops which the pool will add in coinbase
* transaction outputs.
*/
size_t coinbase_output_max_additional_sigops{400};

Note just directly changing defaults in the .capnp file would not be backwards compatible. It would cause problems if a client using the current version of the .capnp file connected to a server using a .capnp file with new defaults, or vice versa. But maybe it would ok to do now if miners are not mixing different releases of Sjors node and client?

(It would also be possible to change defaults in a backwards compatible way adding new fields and renaming the old ones, or adding a new struct and renaming the old one.)

@Sjors
Copy link
Member

Sjors commented Aug 21, 2025 •
edited
Loading

ACK 7392b8b

I also prefer throwing an error to the caller, but clamping is fine.

Note just directly changing defaults in the .capnp file would not be backwards compatible.

For the time being I don't think we should hardcode these values in the .capnp file, since it's hard to change later.

Instead I think we should document the .capnp file, since it's the first thing future IPC client implementers will look at. Maybe we can have a script that automatically populates / updates these docs to follow the interface header?

In Stratum v2 the values are set via the CoinbaseOutputConstraints message:

  • coinbase_output_max_additional_sigops: is a 16 bit integer, so it's can't go over MAX_BLOCK_SIGOPS_COST
  • coinbase_output_max_additional_size: is provided by the pool software and is probably going to be a low sane number, nowhere near the block size limit. The Template Provider (sidecar) IPC client then translates it to block_reserved_weight and just adds 2000.

https://github.com/Sjors/bitcoin/pull/49/files#diff-576b7d75ad2ccce85935852fbc155515ebc3afdfd8fe7b806f6b56c29053b786R234-R236

https://github.com/stratum-mining/sv2-spec/blob/main/07-Template-Distribution-Protocol.md#71-coinbaseoutputconstraints-client---server

@ryanofsky
Copy link
Contributor

For the time being I don't think we should hardcode these values in the .capnp file, since it's hard to change later.

Makes sense, if we want flexibility to change the defaults over time, they should not be set in the .capnp file. Not setting them causes capnp to use 0 as the default.

However, in case 0 is a valid value for either of these options, we may want to set a different, invalid default value like -1 so the server can detect whether the client set the option, and use its own default if the client hasn't filled it in. For c++ clients this doesn't matter, but for python and rust clients and any client not using the c++ struct, this could be important and prevent bugs.

@achow101
Copy link
Member

ACK 7392b8b

@achow101 achow101 merged commit 78351ed into bitcoin:master Aug 21, 2025
19 checks passed
@Sjors
Copy link
Member

Sjors commented Aug 22, 2025

However, in case 0 is a valid value for either of these options, we may want to set a different, invalid default value like -1 so the server can detect whether the client set the option, and use its own default if the client hasn't filled it in.

In that case perhaps it's better if we do hardcode defaults, especially if we can pick safe ones.

@ryanofsky ryanofsky mentioned this pull request Sep 8, 2025
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ryanofsky ryanofsky ryanofsky approved these changes

@Sjors Sjors Awaiting requested review from Sjors

+1 more reviewer

@stickies-v stickies-v stickies-v approved these changes

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
Mining
Projects
None yet
Milestone
30.0
Development

Successfully merging this pull request may close these issues.
7 participants
@sipa @DrahtBot @ryanofsky @Sjors @achow101 @stickies-v @fanquake