Skip to content

rpc: require integer verbosity; remove boolean 'verbose'#33214

Closed
fqlx wants to merge 2 commits into
bitcoin:masterfrom
fqlx:rpc-no-bool-verbosity
Closed

rpc: require integer verbosity; remove boolean 'verbose'#33214
fqlx wants to merge 2 commits into
bitcoin:masterfrom
fqlx:rpc-no-bool-verbosity

Conversation

@fqlx

@fqlx fqlx commented Aug 19, 2025

Copy link
Copy Markdown

Summary

Standardize RPC verbosity to integers only and remove boolean handling for clarity, consistency, and future extensibility.

Rationale

  • Legacy cleanup: Boolean verbosity has been discouraged/deprecated in docs since 2017 and continues to create tech debt (special-case parsing, inconsistent tests, user confusion).
  • Consistency: Integers enable multi-level output without overloading a boolean.

User-visible changes

  • getblock, getrawtransaction, getorphantxs no longer accept booleans.
  • Passing true/false now errors with: Verbosity was boolean but only integer allowed.
  • Migration: false → 0, true → 1.

Code / Docs

  • ParseVerbosity(arg, default) now rejects booleans (removed allow_bool).
  • Call sites and functional tests updated to use integer levels.
  • Developer notes updated; release notes added.

Breaking change: scripts/tools using boolean verbosity must switch to integers.

@DrahtBot

DrahtBot commented Aug 19, 2025

Copy link
Copy Markdown
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/33214.

Reviews

See the guideline for information on the review process.

Type Reviewers
Approach NACK stickies-v

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #bitcoin-core/gui/911 (Adds non-mempool wallet balance to overview by ajtowns)
  • #33892 (policy: Remove individual transaction <minrelay restriction by instagibbs)
  • #33671 (wallet: Add separate balance info for non-mempool wallet txs by ajtowns)
  • #33629 (Cluster mempool by sdaftuar)
  • #33199 (fees: enable CBlockPolicyEstimator return sub 1 sat/vb fee rate estimates by ismaelsadeeq)
  • #32545 (Replace cluster linearization algorithm with SFL by sipa)
  • #32468 (rpc: generateblock to allow multiple outputs by polespinasa)
  • #32138 (wallet, rpc: remove settxfee and paytxfee by polespinasa)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

@fqlx fqlx changed the title Rpc no bool verbosity rpc: require integer verbosity; remove boolean 'verbose Aug 19, 2025
@fqlx fqlx changed the title rpc: require integer verbosity; remove boolean 'verbose rpc: require integer verbosity; remove boolean 'verbose' Aug 19, 2025
@fqlx fqlx force-pushed the rpc-no-bool-verbosity branch from 06f5504 to 97a2eca Compare August 19, 2025 03:04
@fqlx fqlx force-pushed the rpc-no-bool-verbosity branch 3 times, most recently from dd06f31 to 5378c20 Compare August 19, 2025 06:36

@stickies-v stickies-v left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Directionally, this is the right way to go, but I don't think we should be breaking the API for just this change. Let's do these cleanups when we can bundle them with other, necessary breaking changes, on a case-by-case basis. Approach NACK.

@fqlx fqlx force-pushed the rpc-no-bool-verbosity branch from 5378c20 to 66a2640 Compare August 19, 2025 15:11
- Remove boolean handling from ParseVerbosity() and update callers.
- getblock/getrawtransaction/getrawmempool no longer accept true/false.
- Switch functional/Qt tests to integer verbosity.
- Update release notes and docs.
@maflcko

maflcko commented Aug 28, 2025

Copy link
Copy Markdown
Member

I expect those to be widely used (not only in the internal tests, as you noticed in the repeated force pushes), so this may or may not be a widely breaking change (depending on how much they are used in active projects).

There is no auto-generated schema or RPC interface, so those projects would have to fix them manually.

No strong opinion, but maybe this can be closed for now, and revisited when there is a schema generator?

@fqlx

fqlx commented Aug 28, 2025

Copy link
Copy Markdown
Author

I expect those to be widely used (not only in the internal tests, as you noticed in the repeated force pushes), so this may or may not be a widely breaking change (depending on how much they are used in active projects).

There is no auto-generated schema or RPC interface, so those projects would have to fix them manually.

No strong opinion, but maybe this can be closed for now, and revisited when there is a schema generator?

In my opinion, this is a significant breaking change - but it's one that should have been done in like 2018 and now the tech debt has built up. The reason I'm suggesting this change is because people are completely unaware of the 0,1,2,3 API spec due to the assumption it's a boolean and it just works. People don't read the docs Lol. And the boolean flag is compounding a legacy spec. It's a fair assumption given the tests are outdated too.

@achow101

Copy link
Copy Markdown
Member

The way to deprecate these properly would be to first require -deprecatedrpc=boolverbose (or similar) which gates the old behavior. Then the following release can remove them.

Comment thread src/common/args.cpp Outdated
@fqlx fqlx force-pushed the rpc-no-bool-verbosity branch 2 times, most recently from 171af8b to e01e104 Compare September 4, 2025 18:51
@fqlx fqlx force-pushed the rpc-no-bool-verbosity branch from e01e104 to 2d9e67c Compare September 13, 2025 17:07
  1. rpc_deprecated.py: Tests that boolean verbosity fails without the deprecation flag
  2. rpc_boolverbose_deprecated.py: Tests that boolean verbosity works with the -deprecatedrpc=boolverbose flag
  3. Updated existing tests: Now expect the new "Boolean verbosity is deprecated" error message
@fqlx fqlx force-pushed the rpc-no-bool-verbosity branch from 2d9e67c to 457a21a Compare September 15, 2025 17:29
@fqlx fqlx requested a review from maflcko September 17, 2025 04:36

@maflcko maflcko left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

not sure where this pull is going, but it could make sense to split out the test changes into a separate pull (replacing the deprecated values with integers and named args), except for one deprecated value for each RPC, so that all code branches remain tests, but most code is using the new way and named args.

Comment thread src/rpc/util.cpp
}
}

bool IsDeprecatedRPCEnabled(const std::string& method)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

this is still moved

assert_raises_rpc_error(-3, "Verbosity was boolean but only integer allowed", node.getorphantxs, verbosity=True)
assert_raises_rpc_error(-3, "Verbosity was boolean but only integer allowed", node.getorphantxs, verbosity=False)
assert_raises_rpc_error(-3, "Boolean verbosity is deprecated", node.getorphantxs, verbosity=True)
assert_raises_rpc_error(-3, "Boolean verbosity is deprecated", node.getorphantxs, verbosity=False)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

seems odd to change the behavior here to allow deprecated values when they were forbidden

if "coinbase" not in vin:
prevout_tx = self.nodes[0].getrawtransaction(
vin["txid"], True)
vin["txid"], 1)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

when passing integral literal values, it makes sense to use named args, otherwise it is not clear what they mean from reading just the code

@DrahtBot

Copy link
Copy Markdown
Contributor

🐙 This pull request conflicts with the target branch and needs rebase.

@DrahtBot

Copy link
Copy Markdown
Contributor

⌛ There hasn't been much activity lately and the patch still needs rebase. What is the status here?

  • Is it still relevant? ➡️ Please solve the conflicts to make it ready for review and to ensure the CI passes.
  • Is it no longer relevant? ➡️ Please close.
  • Did the author lose interest or time to work on this? ➡️ Please close it and mark it with one of the labels 'Up for grabs' or 'Insufficient Review', so that it can be picked up in the future.

@fqlx fqlx closed this Feb 22, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants