Bugfix: RPC: Remove quotes from non-string oneline descriptions

luke-jr commented at 1:07 am on July 22, 2023: member

Various JSON Object parameters had a oneline_description with quote characters. Fix those, and extend rpcdoccheck to detect them.

Also, slightly improve GBT’s oneline description for template_request.

Bugfix: RPC: Remove quotes from non-string oneline descriptions 7c61e9df90

RPC/rpcdoccheck: Error if a oneline_description has a quote for a non-string de319c6175

DrahtBot commented at 1:07 am on July 22, 2023: contributor

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

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	MarcoFalke
Concept ACK	russeree

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:

#27433 (getblocktemplate improvements for segwit and sigops by Sjors)
#27351 (wallet: add seeds argument to importdescriptors by apoelstra)
#27101 (Support JSON-RPC 2.0 when requested by client by pinheadmz)

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.

RPC/Mining: Document template_request better for getblocktemplate 5e3e83b005

luke-jr force-pushed on Jul 22, 2023

DrahtBot added the label CI failed on Jul 22, 2023

DrahtBot removed the label CI failed on Jul 22, 2023

in src/rpc/util.cpp:1126 in 5e3e83b005

1122+        if (m_opts.oneline_description[0] == '\"' && m_type != Type::STR_HEX && m_type != Type::STR && gArgs.GetBoolArg("-rpcdoccheck", DEFAULT_RPC_DOC_CHECK)) {
1123+            throw std::runtime_error{
1124+                strprintf("Internal bug detected: non-string RPC arg \"%s\" quotes oneline_description:\n%s\n%s %s\nPlease report this issue here: %s\n",
1125+                          m_names, m_opts.oneline_description,
1126+                          PACKAGE_NAME, FormatFullVersion(),
1127+                          PACKAGE_BUGREPORT)};

maflcko commented at 5:44 am on July 22, 2023:

nit: Could use STR_INTERNAL_BUG?

0                STR_INTERNAL_BUG(strprintf("non-string RPC arg \"%s\" quotes oneline_description:\n%s",
1                          m_names, m_opts.oneline_description)
2                           
3                          )};

russeree commented at 11:47 am on July 23, 2023:

The above suggestion doesn’t compile without syntax changes due to error: unterminated argument list invoking macro "STR_INTERNAL_BUG" This is because of a missing ) to close off the arguments for STR_INTERNAL_BUG and then there is a loose comma after m_opts.oneline_description. Once these are removed code does compile but the output is not as clean as the current commit due to an extra " on a new line after the name of the oneline_description compared to the current version which doesn’t have this artifact.

maflcko commented at 7:20 am on July 24, 2023:

I’ve modified my suggestion. (Sorry, I don’t compile them)

russeree commented at 7:30 am on July 24, 2023:

No problem just was testing things. The new version compiles and looks better without the newline but it looks like a quote is appended to the m_opts.oneline_description arg.

The current PR displays correctly "options"

maflcko commented at 7:43 am on July 24, 2023:

It should be fine to just drop the ", as they serve no purpose. A newline already follows the message to delimit it from the next lines.

 0diff --git a/src/util/check.cpp b/src/util/check.cpp
 1index 795dce7124..c4d4b0cc28 100644
 2--- a/src/util/check.cpp
 3+++ b/src/util/check.cpp
 4@@ -18,7 +18,7 @@
 5 
 6 std::string StrFormatInternalBug(std::string_view msg, std::string_view file, int line, std::string_view func)
 7 {
 8-    return strprintf("Internal bug detected: \"%s\"\n%s:%d (%s)\n"
 9+    return strprintf("Internal bug detected: %s\n%s:%d (%s)\n"
10                      "%s %s\n"
11                      "Please report this issue here: %s\n",
12                      msg, file, line, func, PACKAGE_NAME, FormatFullVersion(), PACKAGE_BUGREPORT);

russeree commented at 9:52 am on July 24, 2023:

This compiles and looks great even for the existing errors! Modifying this does cause rpc_misc.py to fail test runner. Overall quite nice.

maflcko commented at 1:02 pm on August 17, 2023:

@russeree Do you want to create a follow-up pull with the changes?

russeree commented at 4:30 am on August 18, 2023:

Ref. https://github.com/bitcoin/bitcoin/pull/28291

in src/rpc/mining.cpp:557 in 5e3e83b005

553@@ -554,7 +554,7 @@ static RPCHelpMan getblocktemplate()
554         "    https://github.com/bitcoin/bips/blob/master/bip-0009.mediawiki#getblocktemplate_changes\n"
555         "    https://github.com/bitcoin/bips/blob/master/bip-0145.mediawiki\n",
556         {
557-            {"template_request", RPCArg::Type::OBJ, RPCArg::Default{UniValue::VOBJ}, "Format of the template",
558+            {"template_request", RPCArg::Type::OBJ, RPCArg::Optional::NO, "Format of the template",

maflcko commented at 5:46 am on July 22, 2023:

I think you can remove the IsNull check below? Also, does this need release notes?

luke-jr commented at 6:40 pm on July 25, 2023:

I don’t think there is any currently-supported case where template_request can be omitted. For a new template, the client must indicate it supports segwit with the rules key. For a proposal, there must be data.

maflcko approved

maflcko commented at 5:47 am on July 22, 2023: member

review ACK 5e3e83b005518659a69916c373b808da27e51791

DrahtBot added the label RPC/REST/ZMQ on Jul 22, 2023

russeree commented at 11:25 am on July 23, 2023: contributor

tACK

I applied this PRs logic with masters .oneline_description and was able to trigger the desired error message.
When checking out this PR without any modifications the expected cli help is displayed.

russeree approved

fanquake merged this on Aug 17, 2023

fanquake closed this on Aug 17, 2023

sidhujag referenced this in commit d8ccf7f817 on Aug 17, 2023

mzumsande referenced this in commit 2394314442 on Aug 17, 2023

fanquake referenced this in commit e4a855c4e0 on Aug 18, 2023

fanquake referenced this in commit 337d6f35a2 on Sep 5, 2023

Frank-GER referenced this in commit 0c4ff3e266 on Sep 8, 2023

Frank-GER referenced this in commit 12ebf85b66 on Sep 8, 2023

Retropex referenced this in commit a17140b6e6 on Oct 4, 2023

Retropex referenced this in commit 5f62ed90f6 on Oct 4, 2023

bitcoin locked this on Aug 17, 2024

Bugfix: RPC: Remove quotes from non-string oneline descriptions #28123

Reviews

Conflicts