Page MenuHomePhabricator

Merge #10858: [RPC] Add "warnings" field to getblockchaininfo and unify "warnings" field in get*info RPCs
ClosedPublic

Authored by nakihito on Mar 22 2019, 23:34.

Details

Summary

395cef7 Change getmininginfo errors field to warnings (Andrew Chow)
8502b20 Unify help text for GetWarnings output in get*info RPCs (Andrew Chow)
f77f0e4 Add warnings field to getblockchaininfo (Andrew Chow)

Pull request description:

The `getblockchaininfo` output does not contain the `warnings` field which the `getinfo`, `getmininginfo`, and `getnetworkinfo` RPCs have. It should have it as the errors pertain to the blockchain. This PR adds that field.

This PR also unifies the help text for the `warnings` field and its output position so that all of the `get*info` commands are consistent.

`getnetworkinfo`'s `errors` field is named `warnings`. I did not change this even though it is inconsistent since this naming has been in use for a long time.

Tree-SHA512: 385ab6acfee67fc8816f4d51ab2bd7a623264c7973906dfbab0a171f199e9db16fde19093a5bc3dfbdd4ff5f19d2186b646eb6b3bae0a4d7c9add43650a4a9d9

Backport of PR10858
https://github.com/bitcoin/bitcoin/commit/9a8e9167f2636fdc2fc047dfed1747920b0f047f?diff=unified#diff-a0c8f511d90e83aa9b5857e819ced344R1165

Completes T567

Test Plan

make check
test_runner.py
getblockchaininfo should also produce a "warnings" field
getmininginfo should have a "warnings" field in place of its "errors" field during regular use
getmininginfo should behave as previously (i.e. have an "errors" field) when running bitcoind -deprecatedrpc=getmininginfo

Diff Detail

Repository
rABC Bitcoin ABC
Branch
BackportPR10858
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 5297
Build 8656: Bitcoin ABC Buildbot (legacy)
Build 8655: arc lint + arc unit

Event Timeline

Owners added a reviewer: Restricted Owners Package.Mar 22 2019, 23:34
nakihito edited the test plan for this revision. (Show Details)

Fixed indentation in getblockchaininfo help.

jasonbcox requested changes to this revision.Mar 25 2019, 17:53

The title and summary of this diff are highly confusing. I know it was just a copy from the PR description, but please update all mentions of errors to warnings to match the change in decision they made in the PR (see comments of the original PR).

src/rpc/blockchain.cpp
1238 ↗(On Diff #7805)

If you did these indents yourself, please revert them. If it was the linter, leave as-is.

src/rpc/mining.cpp
267 ↗(On Diff #7805)

These behavior change needs a mention in release notes.

This revision now requires changes to proceed.Mar 25 2019, 17:53
nakihito retitled this revision from Merge #10858: [RPC] Add "errors" field to getblockchaininfo and unify "errors" field in get*info RPCs to Merge #10858: [RPC] Add "warnings" field to getblockchaininfo and unify "warnings" field in get*info RPCs.Mar 25 2019, 18:50
nakihito edited the summary of this revision. (Show Details)
src/rpc/blockchain.cpp
1238 ↗(On Diff #7805)

A similar change was requested by Fabien in a different diff (see https://reviews.bitcoinabc.org/D2711?id=7751#inline-16561).

nakihito marked an inline comment as done.

Updated release notes with new behavoir.

src/rpc/blockchain.cpp
1238 ↗(On Diff #7805)

This is a good idea to fix it, but this time it's not part of the original PR (it was the last time).
You'd better separate it from this diff to keep this backport closer to the PR, this make it easier to review.

Removed indentation fix in getblockchain help.

src/rpc/blockchain.cpp
1238 ↗(On Diff #7805)

Separated the indentation changes to here: https://reviews.bitcoinabc.org/D2737

Fabien requested changes to this revision.Mar 26 2019, 09:27

The deprecation feature is missing from the test plan.

This revision now requires changes to proceed.Mar 26 2019, 09:27
In D2730#64163, @Fabien wrote:

The deprecation feature is missing from the test plan.

Sorry, I didn't specify that case in the test plan. Updated test plan to be more specific.

jasonbcox added inline comments.
doc/release-notes.md
14

Nit: the release notes will render better if you use backticks ` rather than single quotes ' since this file is Markdown.

For example, look how backticks render on Github: https://github.com/Bitcoin-ABC/bitcoin-abc/blob/master/doc/release-notes.md

Changed single quotes to ` in release notes.

Forgot to add `s to warnings.

This revision is now accepted and ready to land.Mar 28 2019, 08:42