miner: clamp options instead of asserting

sipa commented at 8:56 pm on August 19, 2025: member

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.

miner: clamp options instead of asserting 7392b8b084

DrahtBot added the label Mining on Aug 19, 2025

DrahtBot commented at 8:57 pm on August 19, 2025: contributor

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 commented at 9:15 pm on August 19, 2025: member

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

stickies-v approved

stickies-v commented at 2:40 pm on August 21, 2025: contributor

ACK 7392b8b084be14b75536887b7ff216152d0a3307

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 added this to the milestone 30.0 on Aug 21, 2025

fanquake requested review from Sjors on Aug 21, 2025

fanquake requested review from ryanofsky on Aug 21, 2025

ryanofsky approved

ryanofsky commented at 3:00 pm on August 21, 2025: contributor

Code review ACK 7392b8b084be14b75536887b7ff216152d0a3307. 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 commented at 3:23 pm on August 21, 2025: contributor

@Sjors maybe you have thoughts on this?

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

https://github.com/bitcoin/bitcoin/blob/8333aa5302902f6be929c30b3c2b4e91c6583224/src/ipc/capnp/mining.capnp#L40-L41

that match the c++ defaults:

https://github.com/bitcoin/bitcoin/blob/8333aa5302902f6be929c30b3c2b4e91c6583224/src/node/types.h#L40-L49

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 commented at 6:29 pm on August 21, 2025: member

ACK 7392b8b084be14b75536887b7ff216152d0a3307

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 commented at 7:05 pm on August 21, 2025: 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 commented at 8:50 pm on August 21, 2025: member

ACK 7392b8b084be14b75536887b7ff216152d0a3307

achow101 merged this on Aug 21, 2025

achow101 closed this on Aug 21, 2025

Sjors commented at 6:16 am on August 22, 2025: member

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.

miner: clamp options instead of asserting #33222

Code Coverage & Benchmarks

Reviews