rpc: getblock/getrawtransaction/decode*/gettxout fixups #24718

pull jonatack wants to merge 6 commits into bitcoin:master from jonatack:rpc-fixups changing 2 files +33 −32

jonatack commented at 11:59 PM on March 30, 2022: member

In draft to re-verify the changes and see Drahtbot-enabled crossover with other pulls.
rpc: getblock fixups 2a4767fff4
rpc: getrawtransaction fixups ffe6a9f2d6
rpc: decodepsbt fixups 758b7cf0fe
rpc: decodescript fixups 716c3fa6ea
rpc: decoderawtransaction fixups 96b3a9b729
rpc: gettxout fixups 68a041dd12
DrahtBot added the label RPC/REST/ZMQ on Mar 31, 2022
DrahtBot commented at 3:43 AM on March 31, 2022: member


The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.



Conflicts

Reviewers, this pull request conflicts with the following ones:
- #24716 (rpc: Fix documentation assertion for getrawtransaction by laanwj)
- #24661 (refactor: Use clang-tidy syntax for C++ named arguments by fanquake)
- #23599 (Tidy up RPCTxSerializationFlags by MarcoFalke)
- #23319 (rpc: Return fee and prevout (utxos) to getrawtransaction by dougEfresh)
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.
DrahtBot added the label Needs rebase on Mar 31, 2022
DrahtBot commented at 9:31 AM on March 31, 2022: member



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

<sub>Want to unsubscribe from rebase notifications on this pull request? Just convert this pull request to a "draft".</sub>

in src/rpc/rawtransaction.cpp:417 in 68a041dd12

 410 | @@ -411,10 +411,10 @@ static RPCHelpMan decodescript()
 411 |                   "Result of a witness script public key wrapping this redeem script (not returned for types that should not be wrapped)",
 412 |                   {
 413 |                       {RPCResult::Type::STR, "asm", "String representation of the script public key"},
 414 | +                     {RPCResult::Type::STR, "desc", "Inferred descriptor for the script"},
 415 |                       {RPCResult::Type::STR_HEX, "hex", "Hex string of the script public key"},
 416 | -                     {RPCResult::Type::STR, "type", "The type of the script public key (e.g. witness_v0_keyhash or witness_v0_scripthash)"},
 417 |                       {RPCResult::Type::STR, "address", /*optional=*/true, "The Bitcoin address (only if a well-defined address exists)"},
 418 | -                     {RPCResult::Type::STR, "desc", "Inferred descriptor for the script"},
 419 | +                     {RPCResult::Type::STR, "type", "The type (one of: " + GetAllOutputTypes() + ")"},

luke-jr commented at 12:36 AM on April 14, 2022:

I think " of the script public key" is relevant here

jonatack commented at 12:46 PM on April 29, 2022:

I think " of the script public key" is relevant here

Thanks, Luke, I'll keep that in mind when revisiting this.

fanquake commented at 12:23 PM on April 20, 2022: member

In draft to re-verify the changes and see Drahtbot-enabled crossover with other pulls.

What's the status of this? Have the changes been re-verified? Drahtbot has reported the conflicts at least one time.

Looking at the changes, it seems they could be consolidated. Commits like 96b3a9b72934e62c429a29dc85ceccbf75a662dd and 68a041dd12b3bfee1f8308cd3586dccd3efefed4 are doing the same refactor of changing "The type, eg 'pubkeyhash'" to "The type (one of: " + GetAllOutputTypes() + ")" (also in other commits). It probably makes sense to do that all in a single change, rather than spread across 5 commits.
jonatack commented at 12:58 PM on April 20, 2022: member

Yes, this started out as fixes for one RPC, then I saw similar ones across others, and asked myself whether people prefer changes by kind or by RPC, which is another reason it was in draft. I plan to go through this with fresh eyes and either break it down to changes for one RPC or for one kind of change in one or more small, easy-to-review pulls so they don't just sit there for months.
fanquake commented at 11:40 AM on April 29, 2022: member

and either break it down to changes for one RPC or for one kind of change in one or more small

I don't think this needs to be made any more granular. What is the motivation for per-RPC commits?

easy-to-review pulls so they don't just sit there for months.

If you're referencing this PR; it's been in draft since it was opened, and needed rebase for ~ 1 month. So it's not super surprising that it hasn't had much interest / review.
jonatack commented at 12:10 PM on April 29, 2022: member

I'll re-open this differently. It's not going to see any review after the comments here.
jonatack closed this on Apr 29, 2022
luke-jr referenced this in commit 4a6bf1e072 on May 21, 2022
luke-jr referenced this in commit 74c9cc276d on May 21, 2022
luke-jr referenced this in commit 7c6dc70191 on May 21, 2022
luke-jr referenced this in commit 87723adf94 on May 21, 2022
luke-jr referenced this in commit 9571bdc67d on May 21, 2022
luke-jr referenced this in commit 6de19abba2 on May 21, 2022
MarcoFalke referenced this in commit f27d5f6305 on Jul 25, 2022
DrahtBot locked this on Apr 29, 2023

Contributors

Labels

RPC/REST/ZMQ Needs rebase

Linked (view graph)

#25691 RPC: Document "asm" and "hex" fields for scripts & fix getblock help

rpc: getblock/getrawtransaction/decode*/gettxout fixups #24718

Conflicts