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!

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.

…remove globals 260f8da refactor: remove warnings globals (stickies-v) 9c4b0b7 node: update uiInterface whenever warnings updated (stickies-v) b071ad9 introduce and use the generalized `node::Warnings` interface (stickies-v) 20e616f move-only: move warnings from common to node (stickies-v) bed29c4 refactor: remove unnecessary AppendWarning helper function (stickies-v) Pull request description: This PR: - moves warnings from common to the node library and into the node namespace (as suggested in #29845 (comment)) - generalizes the warnings interface to `Warnings::Set()` and `Warnings::Unset()` methods, instead of having a separate function and globals for each warning. As a result, this simplifies the `kernel::Notifications` interface. - removes warnings.cpp from the kernel library - removes warning globals - adds testing for the warning logic Behaviour change introduced: - the `-alertnotify` command is executed for all `KernelNotifications::warningSet` calls, which now also covers the `LARGE_WORK_INVALID_CHAIN` warning - the GUI is updated automatically whenever a warning is (un)set, covering some code paths where it previously wouldn't be, e.g. when `node::AbortNode()` is called, or for the `LARGE_WORK_INVALID_CHAIN` warning Some discussion points: - ~is `const std::string& id` the best way to refer to warnings? Enums are an obvious alternative, but since we need to define warnings across libraries, strings seem like a straightforward solution.~ _edit: updated approach to use `node::Warning` and `kernel::Warning` enums._ ACKs for top commit: achow101: ACK 260f8da ryanofsky: Code review ACK 260f8da. Only change since last review was rebasing TheCharlatan: Re-ACK 260f8da Tree-SHA512: a3fcedaee0d3ad64e9c111aeb30665162f98e0e72acd6a70b76ff2ddf4f0a34da4f97ce353c322a1668ca6ee4d8a81cc6e6d170c5bbeb7a43cffdaf66646b588

c6eca6f doc: add guidance for RPC to developer notes (tdb3) Pull request description: Adds guidance statements to the RPC interface section of the developer notes with examples of when to implement `-deprecatedrpc=`. Wanted to increase awareness of preferred RPC implementation approaches for newer contributors. This implements some of what's discussed in #29912 (comment) Opinions may differ, so please don't be shy. We want to make RPC as solid/safe as possible. Examples of discussions where guidelines/context might have added value: #30212 (comment) #29845 (comment) #30381 (review) #29954 (comment) #30410 (review) #30713 #30381 #29060 (review) ACKs for top commit: l0rinc: ACK c6eca6f fjahr: ACK c6eca6f maflcko: lgtm ACK c6eca6f jonatack: ACK c6eca6f Tree-SHA512: 01a98a8dc0eb91762b225d3278cdb4a5e380ceb7486fd096b4ad9122bed859cea8584d8996d3dce51272fdb792f4a793a1bd1c5445efeb87f0a30f9b6e59a790

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

This was referenced Apr 28, 2024

RPC: Return permitbaremultisig and maxdatacarriersize in getmempoolinfo #29954

Merged

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

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

Merged

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

Merged

tdb3 mentioned this pull request May 19, 2024

doc: add guidance for RPC to developer notes #30142

Merged

stickies-v mentioned this pull request Aug 6, 2024

getnetworkinfo core RPC introduced new field in json object. btcsuite/btcd#2224

Closed

guggero mentioned this pull request Sep 2, 2024

[bug]: Unable to run lnd 0.18.3rc2 with bitcoin core v28.0rc1 lightningnetwork/lnd#9053

Closed

gus4rs mentioned this pull request Oct 5, 2024

ord fails with Bitcoin Core v28.0 ordinals/ord#3982

Closed

DerEwige mentioned this pull request Oct 5, 2024

[bug]: LND v0.18.3 not start with Bitcoin Core v28 lightningnetwork/lnd#9163

Closed

sipa mentioned this pull request Oct 6, 2024

RPC breakage with v28.0 #31039

Closed

DerEwige mentioned this pull request Oct 8, 2024

warnings field changes from string to array in bitcoin core 28 ACINQ/eclair#2921

Closed

wakiyamap mentioned this pull request Oct 8, 2024

Errors occurring in bitcoin28.0 trezor/blockbook#1138

Closed

twofaktor mentioned this pull request Dec 30, 2024

Node details: getblockchaininfo.warnings.trim is not a function after upgrading bitcoind to 28.0 janoside/btc-rpc-explorer#668

Closed

xyephy mentioned this pull request Feb 26, 2025

Upgrade to bitcoin v28 whiveio/whive#12

Open

bitcoin locked and limited conversation to collaborators May 10, 2025

	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

Uh oh!

Conversation

stickies-v commented Apr 10, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Apr 10, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Coverage

Reviews

Conflicts

Uh oh!

DrahtBot commented Apr 10, 2024

Uh oh!

tdb3 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

No warnings:

One warning:

Two warnings:

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

laanwj commented Apr 16, 2024

Uh oh!

maflcko left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TheCharlatan May 6, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stickies-v commented Apr 18, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

maflcko commented Apr 18, 2024

Uh oh!

tdb3 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

stickies-v commented Apr 18, 2024

Uh oh!

DrahtBot commented Apr 18, 2024

Uh oh!

achow101 commented Apr 23, 2024

Uh oh!

tdb3 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stickies-v commented Apr 25, 2024

Uh oh!

achow101 commented May 1, 2024

Uh oh!

tdb3 left a comment

Choose a reason for hiding this comment

Uh oh!

TheCharlatan left a comment

Choose a reason for hiding this comment

Uh oh!

maflcko left a comment

Choose a reason for hiding this comment

Uh oh!

stickies-v commented Apr 10, 2024 •

edited

Loading

DrahtBot commented Apr 10, 2024 •

edited

Loading

tdb3 left a comment •

edited

Loading

TheCharlatan May 6, 2024 •

edited

Loading

stickies-v commented Apr 18, 2024 •

edited

Loading

tdb3 left a comment •

edited

Loading