rpc: replace ELISION references with explicit result fields#34764
Draft
satsfy wants to merge 8 commits intobitcoin:masterfrom
Draft
rpc: replace ELISION references with explicit result fields#34764satsfy wants to merge 8 commits intobitcoin:masterfrom
satsfy wants to merge 8 commits intobitcoin:masterfrom
Conversation
Contributor
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please copy-paste ConflictsReviewers, this pull request conflicts with the following ones:
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. |
Contributor
|
Concept ACK |
4c7610c to
b2e56e3
Compare
Bortlesboat
reviewed
Mar 7, 2026
Bortlesboat
left a comment
There was a problem hiding this comment.
Code Review ACK
Thorough review of the diff. This is a well-structured elimination of RPCResult::Type::ELISION in favor of explicit documentation:
What it does:
- Renames
DecodeTxDoc→TxDocwith a newTxDocOptionsstruct using designated initializers instead of positional booleans — much cleaner API - Creates
GetBlockFields()to deduplicate the getblock verbosity=1/2/3 help text, parameterized by thetxarray result - Creates
ListSinceBlockTxFields()to share field definitions betweentransactionsandremovedarrays inlistsinceblock - Expands all ELISION placeholders into explicit field documentation
Why this matters: ELISION forced users reading bitcoin-cli help getblock to mentally cross-reference different verbosity levels. Now each level is self-contained, which is especially helpful for verbosity=3 where the prevout fields were previously buried behind two levels of "same as above".
Code observations:
TxDocOptionswith default-false members + designated initializers is clean C++20 — callers only name what they enablefee_optionalcontrolling the/*optional=*/param onRPCResultis a nice touchGetBlockFieldstakingRPCResult tx_resultby value +std::moveis correct- The
#include <rpc/rawtransaction_util.h>addition inblockchain.cppis the right include for the newTxDoc/TxDocOptionsdependency
One minor note: the old verbosity=2 getblock ELISION had descriptive text "The transactions in the format of the getrawtransaction RPC. Different from verbosity = 1 'tx' result" — this contextual note is lost, but the explicit fields are self-documenting so arguably better.
This was referenced Mar 8, 2026
This was referenced Mar 8, 2026
b2e56e3 to
b6e6f5d
Compare
Member
|
Currently this doesn't compile as a few brackets are mis-matched: diff --git a/src/rpc/blockchain.cpp b/src/rpc/blockchain.cpp
index 920080d3b9e..44baf03aad6 100644
--- a/src/rpc/blockchain.cpp
+++ b/src/rpc/blockchain.cpp
@@ -793,9 +793,7 @@ static RPCHelpMan getblock()
{
{RPCResult::Type::ELISION, "", "The transactions in the format of the getrawtransaction RPC. Different from verbosity = 1 \"tx\" result"},
{RPCResult::Type::NUM, "fee", /*optional=*/true, "The transaction fee in " + CURRENCY_UNIT + ", omitted if block undo data is not available"},
- }},
- }},
- }},
+ }})},
RPCResult{"for verbosity = 3",
RPCResult::Type::OBJ, "", "",
{
@@ -806,7 +804,9 @@ static RPCHelpMan getblock()
{
{RPCResult::Type::OBJ, "", "",
TxDoc("The transaction id", TxDocOptions{.prevout = true, .fee = true, .fee_optional = true, .hex = true})},
- }})},
+ }},
+ }},
+ }},
},
RPCExamples{
HelpExampleCli("getblock", "\"00000000c937983704a73af28acdec37b049d214adbda81d7e2a3dd146f6ed09\"")
I'm not super thrilled about expanding (an already large) help output e.g. Comparisongoes to but, this is certainly one "solution" to ellisions being difficult for a spec parser to read, so it definitely improves on this front. But doing it at the expense of a worse experience for humans reading I've picked this PR (with the fixup) into #34683 so we can take a look at it all in one place there. |
b6e6f5d to
15e956a
Compare
Author
|
Thanks for the review, will keep supporting the OpenRPC implementation. In the latest push, I fixed bracket balancing and dropped the fee optionality config since it's always optional (see #34702). On the help output getting larger for user, I envision a follow-up with the OpenRPC work that:
|
Author
|
Note for reviewers: the changes in this PR have been incorporated into #34683. I'll keep this open until it's clear which one will move forward. |
Contributor
|
Concept ACK |
added 2 commits
March 11, 2026 11:57
Initially only move skip_type_check there. In the future, more options can be added, without having to touch the constructors.
This prepares the function to be more flexible, when more options are passed in the future.
Member
|
I'd presume it would be possible to return a human readable help text, but use the full structure internally for I've implemented this in #34799. If you agree, you may review it and rebase on top of it. I've also re-named the function to |
Member
|
Perhaps we could approve CI to run in here please? |
For RPCResults, the type may be ELISION, which is confusing and brittle:
* The elision should only affect the help output, not the type.
* The type should be the real type, so that type checks can be run on
it.
Fix this issue by introducing a new print_elision option and using it
in decodepsbt.
This change will ensure that RPCResult::MatchesType is properly run.
Also, this clarifies the RPC output minimally:
```diff
--- a/decodepsbt
+++ b/decodepsbt
@@ -35,7 +35,7 @@ Result:
"inputs" : [ (json array)
{ (json object)
"non_witness_utxo" : { (json object, optional) Decoded network transaction for non-witness UTXOs
- ...
+ ... The layout is the same as the output of decoderawtransaction.
},
"witness_utxo" : { (json object, optional) Transaction output for witness UTXOs
"amount" : n, (numeric) The value in BTC
```
TxDoc() previously only supported wallet field via a bool
in TxDocOptions. Add prevout, fee, and hex boolean fields to
TxDocOptions so callers can opt into each optional section by name:
TxDoc({.prevout = true, .fee = true})
This avoids positional bool arguments and makes each call site
self-documenting. The internal structure switches from a plain
initializer-list return to Cat() calls so optional result vectors
can be conditionally appended.
Help output is unchanged.
The verbosity=2 result for getrawtransaction contains ELISION prose
both at the top level ("Same output as verbosity = 1") and within
each vin entry. These references are readable for humans but do not
provide structured result metadata.
Replace them with the full result structure using Cat() and TxDoc()
with prevout enabled.
getblock defines four verbosity levels. The verbosity=2 and verbosity=3 results use ELISION entries to refer to lower verbosity layouts, even though the only material difference between levels is the shape of the "tx" field. Introduce GetBlockFields(RPCResult tx_result) to build the full block result structure while substituting the appropriate "tx" definition in the correct position. This avoids ELISION prose and also avoids the duplicate "tx" field a naive Cat() approach would produce. Remove the now-unused getblock_vin helper.
The "removed" array in listsinceblock is described with an ELISION entry referring to the "transactions" array layout. This is clear in human-readable help, but it leaves the result structure implicit. Extract ListSinceBlockTxFields() and use it for both arrays so the transaction layout is defined once and represented explicitly in RPCHelpMan metadata.
15e956a to
6184bfb
Compare
maflcko
reviewed
Mar 12, 2026
| * @param[in] opts Selects which optional fields to include | ||
| * | ||
| * @return A vector of RPCResult describing the decoded transaction object | ||
| */ |
Member
There was a problem hiding this comment.
nit in e9072d1: I think the comment is a bit long. The function should be pretty self-explanatory and a very brief comment should be sufficient:
/// Explain the UniValue "decoded" transaction object, some fields are adjusted according to the passed @p opts.| * default to false so callers only need to name the ones they enable: | ||
| * | ||
| * TxDoc({.prevout = true, .hex = true}) | ||
| */ |
Member
There was a problem hiding this comment.
nit in e9072d1: Same here. I think this can be removed. Also, "all default to false" may not be accurate, if the type is std::string or std::optional.
Maybe remove or:
/// Options controlling some fields in TxDoc(). Callers only need to name the ones they enable:
///
/// TxDoc({.prevout = true, .hex = true})(Also changed to the doxygen /// comment format, because the /** is a bit ugly, when the text starts inconsistently and is inconsistently indented)
Member
There was a problem hiding this comment.
Also, in the same commit: It could be easier to review if the commit adding the option also made use of it at the same time. Otherwise, this looks like dead code.
That is, adding prevout option to TxDoc could be done in the commit f225a25, which makes use of it.
The result description for estimaterawfee currently uses ELISION entries to describe repeated structures for fee horizons and bucket ranges. This keeps bitcoin-cli help compact, but leaves the full result layout implicit in RPCHelpMan metadata. Extract FeeRateBucketDoc() and FeeEstimateHorizonDoc() helpers to define the shared result structure once and reuse it for the "short", "medium", and "long" horizons. Use print_elision on repeated fields so bitcoin-cli help remains compact while the structured result metadata stays explicit. No behavior change.
Author
|
In light of @maflcko’s work in #34799, I’m adjusting this PR so the result structure remains explicit in metadata without worsening the human-readable help output. I’ve already updated The goal remains the same: make these result layouts fully specified in code while avoiding unnecessary user-visible help churn. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
7 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Seeking concept ACK. Partially addresses #29912. Motivated by #34683, which exports OpenRPC from existing
RPCHelpManmetadata. Sample OpenRPC.Some RPC help definitions currently rely on
RPCResult::Type::ELISIONentries whose structure is only described in prose. This is concise for human-readable help, but it prevents tools from deriving complete machine-readable result schemas from the metadata.This PR replaces all non-recursive ELISION-based reuses with shared structured definitions, so the result layout is represented directly in
RPCHelpManmetadata instead of only in text.Affected RPCs:
decodepsbtgetrawtransactiongetblocklistsinceblockThe patch also replaces
DecodeTxDoc()withTxDoc()plus aTxDocOptionsstruct, so decoded transaction layouts can be reused without positional booleans and selectively extended withprevout,fee, andhexfields.As a consequence,
bitcoin-clioutput for affected RPCs becomes more verbose, since prose summaries like "Same output as verbosity = 1" are replaced by the full field definitions. The actual RPC return values are unchanged. I opened this as a draft because I'd like to discuss whether this tradeoff is acceptable, or whether the help renderer should collapse some sub-structures via manual configuration back into short summaries while keeping the underlying metadata explicit.This PR doesn't cover
getaddressinfo.embedded, which is recursive — runtime output can contain nested embedded objects, andRPCResulthas no way to express that today. Happy to discuss approaches here if anyone has thoughts.Changes:
DecodeTxDoc()withTxDoc()andTxDocOptionsdecodepsbt,getrawtransaction,getblock, andlistsinceblockTxDoc(),GetBlockFields(), andListSinceBlockTxFields()getblocktransactionfeedocumentation optional where it is conditionally present