rpc: return warnings as an array instead of just a single one #29845

stickies-v · 2024-04-10T10:29:39Z

The RPC documentation for getblockchaininfo, getmininginfo and getnetworkinfo states that "warnings" returns "any network and blockchain warnings". In practice, only a single warning (i.e. the latest one that is set) is returned, the other ones are ignored.

Fix that by returning all warnings as an array.

As a side benefit, clean up the GetWarnings() logic.

Since this PR changes the RPC result schema, I've added release notes. Users can temporarily revert to the old results by using -deprecatedrpc=warnings, until it's removed in a future version.

Some historical context from git log:

when GetWarnings was introduced in 4019262, it was used in the getinfo RPC, where only a single error/warning was returned (similar to how it is now).
later on, "warnings" RPC response fields were introduced, e.g. in ef2a3de, with the description stating that it returned "any network warnings" but in practice still only a single warning was returned

DrahtBot · 2024-04-10T10:29:42Z

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

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	achow101, tdb3, TheCharlatan, maflcko
Concept ACK	laanwj

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

Conflicts

No conflicts as of last run.

DrahtBot · 2024-04-10T11:32:25Z

🚧 At least one of the CI tasks failed. Make sure to run all tests locally, according to the
documentation.

Possibly this is due to a silent merge conflict (the changes in this pull request being
incompatible with the current code in the target branch). If so, make sure to rebase on the latest
commit of the target branch.

Leave a comment here, if you need help tracking down a confusing failure.

_{Debug: https://github.com/bitcoin/bitcoin/runs/23654678747}

tdb3

ACK for 4b82191

Nice addition and great opportunistic cleanup of GetWarnings().

Built and ran all unit and functional tests (all passed).
On signet, tested by running getblockchaininfo with

no warnings, by supressing the pre-release warning with if (false && !CLIENT_VERSION_IS_RELEASE) and rebuilding,
one warning (the pre-release warning)
two warnings, the pre-release warning and date/time warning by manually setting the system clock back two days before starting bitcoind

All three cases seemed to work without issue.
The only difference observed vs master is that when no warnings are present, master returns an empty string of warnings vs this PR returning an empty array. This was touched on in the description of the PR. I agree that adding a -deprecatedrpc option does seem like a bit of overkill. IMHO the release notes addition should help inform users.

Left one minor nit.

No warnings:

dev@bdev01:~/bitcoin$ src/bitcoin-cli -signet getblockchaininfo
{
  "chain": "signet",
  "blocks": 0,
  "headers": 20634,
  "bestblockhash": "00000008819873e925422c1ff0f99f7cc9bbb232af63a077a480a3633bee1ef6",
  "difficulty": 0.001126515290698186,
  "time": 1598918400,
  "mediantime": 1598918400,
  "verificationprogress": 4.029370408440712e-07,
  "initialblockdownload": true,
  "chainwork": "000000000000000000000000000000000000000000000000000000000049d414",
  "size_on_disk": 7872875,
  "pruned": false,
  "warnings": [
  ]
}

One warning:

dev@bdev01:~/bitcoin$ src/bitcoin-cli -signet getblockchaininfo
{
  "chain": "signet",
  "blocks": 0,
  "headers": 7358,
  "bestblockhash": "00000008819873e925422c1ff0f99f7cc9bbb232af63a077a480a3633bee1ef6",
  "difficulty": 0.001126515290698186,
  "time": 1598918400,
  "mediantime": 1598918400,
  "verificationprogress": 4.029367626439888e-07,
  "initialblockdownload": true,
  "chainwork": "000000000000000000000000000000000000000000000000000000000049d414",
  "size_on_disk": 2890410,
  "pruned": false,
  "warnings": [
    "This is a pre-release test build - use at your own risk - do not use for mining or merchant applications"
  ]
}

Two warnings:

dev@bdev01:~/bitcoin$ src/bitcoin-cli -signet getblockchaininfo
{
  "chain": "signet",
  "blocks": 0,
  "headers": 30645,
  "bestblockhash": "00000008819873e925422c1ff0f99f7cc9bbb232af63a077a480a3633bee1ef6",
  "difficulty": 0.001126515290698186,
  "time": 1598918400,
  "mediantime": 1598918400,
  "verificationprogress": 4.032686769779725e-07,
  "initialblockdownload": true,
  "chainwork": "000000000000000000000000000000000000000000000000000000000049d414",
  "size_on_disk": 12258274,
  "pruned": false,
  "warnings": [
    "This is a pre-release test build - use at your own risk - do not use for mining or merchant applications",
    "Please check that your computer's date and time are correct! If your clock is wrong, Bitcoin Core will not work properly."
  ]
}

tdb3 · 2024-04-15T01:16:49Z

src/warnings.h

- * @returns the warning string
- */
-bilingual_str GetWarnings(bool verbose);
+/** Return potential problems detected by the node. */


nit: Looks like the old function comment header used Doxygen syntax. Would recommend keeping this style, but feel free to disregard this nit since the return type seems straightforward, and the function is small/digestible.

diff --git a/src/warnings.h b/src/warnings.h index 0b8eb2c9a1..4ae05d4862 100644 --- a/src/warnings.h +++ b/src/warnings.h @@ -12,7 +12,10 @@ struct bilingual_str; void SetMiscWarning(const bilingual_str& warning); void SetfLargeWorkInvalidChainFound(bool flag); -/** Return potential problems detected by the node. */ +/** Return potential problems detected by the node. + * + * @returns each warning string as a vector element. + */ std::vector<bilingual_str> GetWarnings(); #endif // BITCOIN_WARNINGS_H

I generally tend to prefer to doxygen-ify documentation, but in this case I'm not sure the extra line adds anything over the return type and function name, so then is it just unnecessary boilerplate that makes the documentation less instead of more clear? No strong view either way, so will leave this open.

laanwj · 2024-04-16T06:31:33Z

This is something that we should have done a long time ago imo. I wouldn't be surprised if I have an ancient PR doing just this.
Concept ACK!

maflcko

Left some stupid style nits. Leave a comment if you want to ignore them or address them.

very nice. Thanks for fixing this. ACK 4b82191 🛏

Show signature

Signature:

untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: very nice. Thanks for fixing this. ACK 4b821915bf92082043d6cf60efb1a5faea0151db 🛏
2C4/MGb7Zt2VKd0SexvYcgZQplu/bak1Sxc/jU7RHsNG2P6rqMRriRgrMxJ5xP/EzEPad6wnaHJycRWZEhfyDA==

src/rpc/mining.cpp

src/rpc/net.cpp

src/rpc/util.cpp

maflcko · 2024-04-18T06:28:44Z

src/warnings.h

- */
-bilingual_str GetWarnings(bool verbose);
+/** Return potential problems detected by the node. */
+std::vector<bilingual_str> GetWarnings();

 #endif // BITCOIN_WARNINGS_H


unrelated follow-up idea: It seems wrong to have the warnings module in the common library, when it is using globals to keep track of the warnings.

I guess it would be nice to actually move it to the node library, and node namespace?

Agreed, that looks like a nice follow-up. Happy to have a go after this gets merged.

Happy to have a go after this gets merged.

Implemented in master...stickies-v:bitcoin:2024-04/move-warnings-node

I think it is better off in the kernel library eventually: https://github.com/bitcoin/bitcoin/pull/28690/files#diff-4cb884d03ebb901069e4ee5de5d02538c40dd9b39919c615d8eaa9d364bbbd77R735

kernel

I guess it is a bit odd to have g_timeoffset_warning in the kernel, but happy to review either approach.

I guess it would be nice to actually move it to the node library, and node namespace?

Done in #30058

stickies-v · 2024-04-18T07:50:02Z

Thanks for the reviews everyone. Force-pushed to address maflcko's style nits. Otherwise no changes so should be a quick re-review.

CI failure unrelated: #29831

maflcko · 2024-04-18T08:36:39Z

scanblocks CI failure is known, unrelated, and can be ignored.

re-ACK 7cad21a 🛵

Show signature

Signature:

untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: re-ACK 7cad21a8bbd1e0b64a5e586b405c8619b9e701a5 🛵
GVLbcvjQpk/x+zLWutUD7wboY/bIbcwsBYm7+oHZsJpP5GtA9qrMzRn8p2XCYVPsb0tNAcG1G0afsp8e+M4/Ag==

tdb3

ltgm.
cr re ACK for 7cad21a

src/rpc/blockchain.cpp

stickies-v · 2024-04-18T19:20:51Z

Force pushed to add -deprecatedrpc=warnings to address review concerns.

DrahtBot · 2024-04-18T19:38:31Z

🚧 At least one of the CI tasks failed. Make sure to run all tests locally, according to the
documentation.

Possibly this is due to a silent merge conflict (the changes in this pull request being
incompatible with the current code in the target branch). If so, make sure to rebase on the latest
commit of the target branch.

Leave a comment here, if you need help tracking down a confusing failure.

_{Debug: https://github.com/bitcoin/bitcoin/runs/23994226471}

achow101 · 2024-04-23T19:44:42Z

ACK 7b64820

tdb3

Approach ACK.

Re-ran the sanity checks in #29845 (review)
All were successful.

Tried testing out the -deprecatedrpc=warnings functionality. I'm guessing I'm missing something simple/trivial (or improperly using -deprecatedrpc). When running src/bitcoind -deprecatedrpc=warnings, both curl and bitcoin-cli returned an error:

curl:

$ curl --user __cookie__ --data-binary '{"id": "curltest", "method": "getblockchaininfo", "params": []}' -H 'content-type: text/plain;' http://127.0.0.1:38332/
Enter host password for user '__cookie__':
{"result":null,"error":{"code":-1,"message":"Internal bug detected: RPC call \"getblockchaininfo\" returned incorrect type:\n{\n    \"warnings\": \"returned type is string, but declared as array in doc\"\n}\nBitcoin Core v27.99.0-7b64820da65d-dirty\nPlease report this issue here: https://github.com/bitcoin/bitcoin/issues\n"},"id":"curltest"}

bitcoin-cli:

$ src/bitcoin-cli -signet getblockchaininfo
error code: -1
error message:
Internal bug detected: RPC call "getblockchaininfo" returned incorrect type:
{
    "warnings": "returned type is string, but declared as array in doc"
}
Bitcoin Core v27.99.0-7b64820da65d-dirty
Please report this issue here: https://github.com/bitcoin/bitcoin/issues

I did some quick searching and also checked for -help output for "deprecatedrpc" for usage (both bitcoind and bitcoin-cli), but didn't see anything indicating appropriate usage.

stickies-v · 2024-04-25T12:06:48Z

both curl and bitcoin-cli returned an error:

I suspect you're running with -rpcdoccheck, you shouldn't get any errors without it? I'm not sure if -rpcdoccheck is meant to be an internal-only testing flag (as per #24695), but it doesn't seem to be documented as such, so I guess better to be safe - force pushed to make the docs contingent on the -rpcdeprecated too.

Thanks for your diligent re-review!

tdb3 · 2024-04-27T17:27:39Z

both curl and bitcoin-cli returned an error:

I suspect you're running with -rpcdoccheck, you shouldn't get any errors without it? I'm not sure if -rpcdoccheck is meant to be an internal-only testing flag (as per #24695), but it doesn't seem to be documented as such, so I guess better to be safe - force pushed to make the docs contingent on the -rpcdeprecated too.

Thanks for your diligent re-review!

ACK for 07cb2ca

I hadn't explicitly been running with -rpcdoccheck, but will look more into it, thanks. Re-ran the tests above (#29845 (review) and #29845 (review)). All worked as expected (below is the output when running curl and bitcoin-cli with src/bitcoind -deprecatedrpc=warnings).

$ curl --user __cookie__ --data-binary '{"id": "curltest", "method": "getblockchaininfo", "params": []}' -H 'content-type: text/plain;' http://127.0.0.1:38332/
Enter host password for user '__cookie__':
{"result":{"chain":"signet","blocks":193052,"headers":193114,"bestblockhash":"00000089592fbaa28b1db9da72361ad09c9b7f743fed75918720d8941b2ce2a4","difficulty":0.003080949497532796,"time":1714194156,"mediantime":1714191581,"verificationprogress":0.9998987458318408,"initialblockdownload":false,"chainwork":"0000000000000000000000000000000000000000000000000000022ea328e71f","size_on_disk":1342889501,"pruned":false,"warnings":"This is a pre-release test build - use at your own risk - do not use for mining or merchant applications"},"error":null,"id":"curltest"}

$ src/bitcoin-cli -signet getblockchaininfo
{
  "chain": "signet",
  "blocks": 193052,
  "headers": 193114,
  "bestblockhash": "00000089592fbaa28b1db9da72361ad09c9b7f743fed75918720d8941b2ce2a4",
  "difficulty": 0.003080949497532796,
  "time": 1714194156,
  "mediantime": 1714191581,
  "verificationprogress": 0.9998987116639849,
  "initialblockdownload": false,
  "chainwork": "0000000000000000000000000000000000000000000000000000022ea328e71f",
  "size_on_disk": 1342889501,
  "pruned": false,
  "warnings": "This is a pre-release test build - use at your own risk - do not use for mining or merchant applications"
}

The RPC documentation for `getblockchaininfo`, `getmininginfo` and `getnetworkinfo` states that "warnings" returns "any network and blockchain warnings". In practice, only a single warning is returned. Fix that by returning all warnings as an array. As a side benefit, cleans up the GetWarnings() logic.

achow101 · 2024-05-01T20:10:44Z

re-ACK 42fb531

tdb3

Re ACK for 42fb531

Ran the tests in #29845 (comment) and #29845 (review) again (no warnings, one warning, two warnings, and with -deprecatedrpc with one warning). All behaved as expected.

TheCharlatan

ACK 42fb531

maflcko

ACK 42fb531 🔺

Show signature

Signature:

untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: ACK 42fb5311b19582361409d65c6fddeadbee14bb97 🔺
DkZcGaegmeNqao4OyqKkZvPK1NSWofgTqWxVDrmqPuY4EZlSajPN8iz8kpunlYESlSl31ykCX/U9HS/4bHoUCQ==

maflcko · 2024-05-06T09:14:36Z

doc/release-notes-29845.md

+- the `warnings` field in `getblockchaininfo`, `getmininginfo` and
+  `getnetworkinfo` now returns all the active node warnings as an array
+  of strings, instead of just a single warning. The current behaviour
+  can temporarily be restored by running bitcoind with configuration


Suggested change

can temporarily be restored by running bitcoind with configuration

can temporarily be restored by running with configuration

nit (only if you re-touch): Could remove the executable name, as it can be bitcoin-qt as well, or a different name.

DrahtBot added the RPC/REST/ZMQ label Apr 10, 2024

DrahtBot added the CI failed label Apr 10, 2024

stickies-v force-pushed the 2024-04/make-warnings-arr branch 2 times, most recently from 7b40542 to 4b82191 Compare April 10, 2024 11:41

DrahtBot removed the CI failed label Apr 10, 2024

DrahtBot mentioned this pull request Apr 10, 2024

Simplify network-adjusted time warning logic #29623

Merged

tdb3 reviewed Apr 15, 2024

View reviewed changes

maflcko approved these changes Apr 18, 2024

View reviewed changes

DrahtBot requested a review from laanwj April 18, 2024 06:33

stickies-v force-pushed the 2024-04/make-warnings-arr branch from 4b82191 to 7cad21a Compare April 18, 2024 07:46

DrahtBot requested a review from tdb3 April 18, 2024 08:36

tdb3 reviewed Apr 18, 2024

View reviewed changes

achow101 reviewed Apr 18, 2024

View reviewed changes

src/rpc/blockchain.cpp Outdated Show resolved Hide resolved

stickies-v force-pushed the 2024-04/make-warnings-arr branch from 7cad21a to 60b4334 Compare April 18, 2024 19:19

stickies-v force-pushed the 2024-04/make-warnings-arr branch from 60b4334 to 7b64820 Compare April 18, 2024 19:38

DrahtBot added the CI failed label Apr 18, 2024

DrahtBot removed the CI failed label Apr 18, 2024

DrahtBot requested review from tdb3 and maflcko April 23, 2024 19:44

tdb3 reviewed Apr 25, 2024

View reviewed changes

stickies-v force-pushed the 2024-04/make-warnings-arr branch from 7b64820 to 07cb2ca Compare April 25, 2024 11:50

DrahtBot requested a review from achow101 April 27, 2024 17:27

This was referenced Apr 28, 2024

RPC: Return permitbaremultisig and maxdatacarriersize in getmempoolinfo #29954

Open

RFC: Formal description of the RPC API #29912

Open

DrahtBot added the Needs rebase label Apr 30, 2024

stickies-v force-pushed the 2024-04/make-warnings-arr branch from 07cb2ca to b716e02 Compare May 1, 2024 12:06

DrahtBot removed the Needs rebase label May 1, 2024

stickies-v force-pushed the 2024-04/make-warnings-arr branch from b716e02 to 42fb531 Compare May 1, 2024 13:45

DrahtBot requested a review from tdb3 May 1, 2024 20:10

tdb3 reviewed May 3, 2024

View reviewed changes

TheCharlatan approved these changes May 6, 2024

View reviewed changes

maflcko approved these changes May 6, 2024

View reviewed changes

achow101 merged commit 63d0b93 into bitcoin:master May 6, 2024
16 checks passed

stickies-v deleted the 2024-04/make-warnings-arr branch May 6, 2024 23:00

stickies-v mentioned this pull request May 7, 2024

Encapsulate warnings in generalized node::Warnings and remove globals #30058

Open

0xB10C mentioned this pull request May 8, 2024

Allow warnings to be either a string or a string sequence. rust-bitcoin/rust-bitcoincore-rpc#353

Merged

ryanofsky mentioned this pull request May 13, 2024

kernel: Streamline util library #29015

Open

tdb3 mentioned this pull request May 19, 2024

doc: add guidance for RPC to developer notes #30142

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

rpc: return warnings as an array instead of just a single one #29845

rpc: return warnings as an array instead of just a single one #29845

stickies-v commented Apr 10, 2024 •

edited

DrahtBot commented Apr 10, 2024 •

edited

DrahtBot commented Apr 10, 2024

tdb3 left a comment •

edited

tdb3 Apr 15, 2024

stickies-v Apr 15, 2024

laanwj commented Apr 16, 2024

maflcko left a comment

maflcko Apr 18, 2024

stickies-v Apr 18, 2024

stickies-v Apr 29, 2024

TheCharlatan May 6, 2024 •

edited

maflcko May 6, 2024

stickies-v May 7, 2024

stickies-v commented Apr 18, 2024 •

edited

maflcko commented Apr 18, 2024

tdb3 left a comment

stickies-v commented Apr 18, 2024

DrahtBot commented Apr 18, 2024

achow101 commented Apr 23, 2024

tdb3 left a comment •

edited

stickies-v commented Apr 25, 2024

tdb3 commented Apr 27, 2024

achow101 commented May 1, 2024

tdb3 left a comment

TheCharlatan left a comment

maflcko left a comment

maflcko May 6, 2024

	can temporarily be restored by running bitcoind with configuration
	can temporarily be restored by running with configuration

rpc: return warnings as an array instead of just a single one #29845

rpc: return warnings as an array instead of just a single one #29845

Conversation

stickies-v commented Apr 10, 2024 • edited

DrahtBot commented Apr 10, 2024 • edited

Code Coverage

Reviews

Conflicts

DrahtBot commented Apr 10, 2024

tdb3 left a comment • edited

Choose a reason for hiding this comment

No warnings:

One warning:

Two warnings:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

laanwj commented Apr 16, 2024

maflcko left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TheCharlatan May 6, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stickies-v commented Apr 18, 2024 • edited

maflcko commented Apr 18, 2024

tdb3 left a comment

Choose a reason for hiding this comment

stickies-v commented Apr 18, 2024

DrahtBot commented Apr 18, 2024

achow101 commented Apr 23, 2024

tdb3 left a comment • edited

Choose a reason for hiding this comment

stickies-v commented Apr 25, 2024

tdb3 commented Apr 27, 2024

achow101 commented May 1, 2024

tdb3 left a comment

Choose a reason for hiding this comment

TheCharlatan left a comment

Choose a reason for hiding this comment

maflcko left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stickies-v commented Apr 10, 2024 •

edited

DrahtBot commented Apr 10, 2024 •

edited

tdb3 left a comment •

edited

TheCharlatan May 6, 2024 •

edited

stickies-v commented Apr 18, 2024 •

edited

tdb3 left a comment •

edited