[RPC] Add "errors" field to getblockchaininfo and unify "errors" field in get*info RPCs #10858

achow101 · 2017-07-17T23:14:34Z

The getblockchaininfo output does not contain the errors 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 errors 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.

laanwj · 2017-07-18T06:34:45Z

IMO, errors does not need to be duplicated on every get*info call. It's fine to have it on only one.

Also, "warnings" is a better name. None of the messages are really errors, all of them are advisory. Errors result in a node abort. This is why the old name (on the deprecated getinfo) is "errors" and the new one (on getnetworkinfo) is named "warnings".
(having it on getmininginfo is weird, though, I didn't even know that)

achow101 · 2017-07-18T20:18:20Z

I have changed the field to be warnings instead of errors. However I left getmininginfo's errors field as is so that we don't break api compatibility.

jnewbery · 2017-09-01T19:48:48Z

I think we can be more aggressive in improving and harmonizing RPCs, as long as we give plenty of warning and time for clients to upgrade. How about changing getmininginfo to return a warning field in the next version (currently 0.16), but add a command line argument to have it return the information in an errors field. Then remove that option in the following version.

achow101 · 2017-09-19T02:03:08Z

Rebased.

@jnewbery I don't like the idea of adding special command line args for dictating whether something is shown in a specific RPC.

jnewbery · 2017-09-19T13:41:03Z

@achow101 - do you disagree with changing RPCs in general? I think we need to have some way of evolving the API over releases otherwise mistakes made in the past are set in stone forever.

If you do agree that the API needs to change over releases, can you suggest a better, safer way of doing it than making the versioning configurable with a command line option? We could obviously add a version path to the RPC endpoint like many REST APIs implement, but I think that's overkill at this stage (and I don't know of anyone who's volunteered implementing it)

achow101 · 2017-09-19T13:55:07Z

@jnewbery I agree that the API should change. However I don't think that we should give each deprecated thing its own command line argument to enable or disable it. I think we could do some sort of versioning with different endpoints like what multiwallet does with multiple wallets. So we could add a /deprecated/ endpoint for which any deprecated behavior would exist behind before being removed in the future. If you want to access deprecated stuff, you can use that endpoint, and ideally, non-deprecated stuff would also work behind that endpoint as well.

jnewbery · 2017-09-19T14:16:36Z

we could add a /deprecated/ endpoint for which any deprecated behavior would exist behind before being removed in the future. If you want to access deprecated stuff, you can use that endpoint, and ideally, non-deprecated stuff would also work behind that endpoint as well.

I don't think that's ideal. The user would then:

upgrade, find that an RPC that they use is deprecated and then have to update their client software to access the new endpoint. Updating client software is more difficult than updating a Bitcoin Core command line option, especially for businesses where the client software might be maintained by a different team.
then continue using the /deprecated endpoint for everything, because why wouldn't they? At that point we've lost all benefit of having a staged end-of-life for deprecated RPCs.

achow101 · 2017-09-19T14:24:58Z

@jnewbery Ok, then.. How about a -deprecated=<rpc> option? So instead of a switch for every deprecated thing, the user can specify that some or all RPCs should use the deprecated behavior.

jnewbery · 2017-09-19T15:30:30Z

How about a -deprecated= option?

Yes, sounds sensible. I think -deprecatedrpcmethod is probably a bit more descriptive. We could just rename the new option from #11031 .

achow101 · 2017-09-26T16:05:50Z

Rebased onto #11031 to use -deprecatedrpc for showing errors instead of warnings for getmininginfo. I wasn't sure what to do with the help text, so I just left the errors field there with a deprecation warning.

jnewbery · 2017-09-26T16:33:18Z

p2p-versionbits-warning.py requires update to use ['warnings'] instead of ['errors']. Otherwise looks good to me.

achow101 · 2017-09-26T17:43:15Z

Fixed p2p-versionbits-warning.py

Changes the errors field to warnings. To maintain compatibility, the errors field is deprecated and enabled by starting bitcoind with -deprecatedrpc=getmininginfo

achow101 · 2017-09-27T15:27:33Z

Rebased to master

jnewbery · 2017-09-27T20:26:52Z

Tested ACK 395cef7

jonasschnelli · 2017-09-28T03:12:52Z

src/rpc/blockchain.cpp

@@ -1162,6 +1163,7 @@ UniValue getblockchaininfo(const JSONRPCRequest& request)
            "        }\n"
            "     }\n"
            "  }\n"
+            "  \"warnings\" : \"...\",         (string) any network and blockchain warnings.\n"


nit: I think here is one whitespace missing between text warning[...]and description ((string)[...])

jonasschnelli · 2017-09-28T03:34:22Z

utACK 395cef7

… "errors" field in get*info RPCs 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 `errors` 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 `errors` 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

db1cbcc [RPC] Remove deprecated addmultisigaddress return format (John Newbery) cb28a0b [RPC] Remove deprecated createmultisig object (John Newbery) ed45c82 [tests] Remove test for deprecated createmultsig option (John Newbery) d066a1c [rpc] Remove deprecated getmininginfo RPC option (John Newbery) c6f09c2 [rpc] remove deprecated estimatefee RPC (John Newbery) a8e437a [tests] Remove estimatefee from rpc_deprecated.py test (John Newbery) a5623b1 [tests] Remove tests for deprecated estimatefee RPC (John Newbery) d119f2e [tests] Fix style warnings in feature_fee_estimation.py (John Newbery) Pull request description: There were some RPC/RPC options deprecated in v0.16. Those can now be removed from master since v0.16 has been branched. - `estimatefee` RPC has been removed. The `feature_fee_estimation.py` test has been updated to remove the RPC, but doesn't yet have good coverage of the replacement RPC `estimatesmartfee`. Improving the test coverage should be done in a new PR. (#11031) - the `errors` field returned by `getmininginfo` has been deprecated and replaced by a `warning` field. (#10858) - providing addresses as inputs to `createmultisig` has been deprecated. Users should use `addmultisigaddress` instead (#11415) - The return format from `addmultisigaddress` has changed (#11415) `getwitnessaddress` was also deprecated in v0.16 and can be removed, but many tests are using that RPC, so it's a larger job to remove. It should be removed in a separate PR (possibly after #11739 and #11398 have been merged and the segwit test code tidied up) Tree-SHA512: 8ffaa5f6094131339b9e9e468e8b141de4b144697d2271efa2992b80b12eb97849ade3da8df5c1c9400ed4c04e6a029926550a3e5846d2029b644f9e84ac7124

…d unify "errors" field in get*info RPCs 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 `errors` 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 `errors` 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

db1cbcc [RPC] Remove deprecated addmultisigaddress return format (John Newbery) cb28a0b [RPC] Remove deprecated createmultisig object (John Newbery) ed45c82 [tests] Remove test for deprecated createmultsig option (John Newbery) d066a1c [rpc] Remove deprecated getmininginfo RPC option (John Newbery) c6f09c2 [rpc] remove deprecated estimatefee RPC (John Newbery) a8e437a [tests] Remove estimatefee from rpc_deprecated.py test (John Newbery) a5623b1 [tests] Remove tests for deprecated estimatefee RPC (John Newbery) d119f2e [tests] Fix style warnings in feature_fee_estimation.py (John Newbery) Pull request description: There were some RPC/RPC options deprecated in v0.16. Those can now be removed from master since v0.16 has been branched. - `estimatefee` RPC has been removed. The `feature_fee_estimation.py` test has been updated to remove the RPC, but doesn't yet have good coverage of the replacement RPC `estimatesmartfee`. Improving the test coverage should be done in a new PR. (bitcoin#11031) - the `errors` field returned by `getmininginfo` has been deprecated and replaced by a `warning` field. (bitcoin#10858) - providing addresses as inputs to `createmultisig` has been deprecated. Users should use `addmultisigaddress` instead (bitcoin#11415) - The return format from `addmultisigaddress` has changed (bitcoin#11415) `getwitnessaddress` was also deprecated in v0.16 and can be removed, but many tests are using that RPC, so it's a larger job to remove. It should be removed in a separate PR (possibly after bitcoin#11739 and bitcoin#11398 have been merged and the segwit test code tidied up) Tree-SHA512: 8ffaa5f6094131339b9e9e468e8b141de4b144697d2271efa2992b80b12eb97849ade3da8df5c1c9400ed4c04e6a029926550a3e5846d2029b644f9e84ac7124

d5526bd Wrap dumpwallet warning and note scripts aren't dumped (MeshCollider) 3711c6a Add wallet backup text to import*, add* and dumpwallet RPCs (MeshCollider) dbda874 [Wallet] always show help-line of wallet encryption calls (Jonas Schnelli) 20c269b Avoid opening copied wallet databases simultaneously (Russell Yanofsky) e411b70 [wallet] Fix leak in CDB constructor (João Barbosa) f15aeea Change getmininginfo errors field to warnings (Andrew Chow) c04390b Unify help text for GetWarnings output in get*info RPCs (random-zebra) 1d966ce Add warnings field to getblockchaininfo (Andrew Chow) ffcd781 [Trivial] Cleanup after MOVE-ONLY commits (random-zebra) e067235 MOVEONLY: Init functions wallet/wallet.cpp -> wallet/init.cpp (random-zebra) e947eec MOVEONLY: Fee functions wallet/wallet.cpp -> wallet/fees.cpp (random-zebra) 2188c3e Move some static functions out of wallet.h/cpp (random-zebra) f49acf7 [wallet] [moveonly] Move CAffectedKeysVisitor (random-zebra) 8bd979f [wallet] Specify wallet name in wallet loading errors (random-zebra) 900bbfa Reject invalid wallet files (João Barbosa) a1f4e2a Reject duplicate wallet filenames (random-zebra) ee52c2e Fix misleading "Method not found" multiwallet errors (Russell Yanofsky) ce35e1e [Qt] Use wallet 0 in rpc console if running with multiple wallets (random-zebra) 37089d1 [tests] Update wallet_multiwallet.py functional test (random-zebra) 3955ee9 [Doc] Update release notes (random-zebra) 4fd5913 [wallet] [rpc] Add listwallets RPC (John Newbery) 1525281 [wallet] [rpc] print wallet name in getwalletinfo (John Newbery) fdf5da0 [wallet] fix comment for CWallet::Verify() (John Newbery) cf4a90b Remove factor of 3 from definition of dust. (random-zebra) a1c56fd [Policy] Introduce -dustrelayfee (random-zebra) 9fb29cc [Doc] Update multiwallet release notes (random-zebra) 379255e [Tests][Trivial] Add wallet_multiwallet.py to test_runner (random-zebra) 808fbc3 [Bugfix] consider boolean value of -zapwallettxes ParameterInteraction (random-zebra) f9141f8 [QA] Add wallet_multiwallet.py functional test (John Newbery) 2e02006 Rename -usewallet to -rpcwallet (Alex Morcos) a64440b Select wallet based on the given endpoint (Jonas Schnelli) 5683a9c Complete previous commit by moving mn stuff out of libbitcoin_wallet (random-zebra) b0fe92f Fix test_pivx circular dependency issue (random-zebra) 6cb2b92 Add wallet endpoint support to bitcoin-cli (-usewallet) (Jonas Schnelli) 7dd3916 Register wallet endpoint (Jonas Schnelli) 5bd1bd7 Properly forbid -salvagewallet and -zapwallettxes for multi wallet. (Alex Morcos) 41a7335 Remove unused variables (practicalswift) 5c3d73f Avoid CWalletTx copies in GetAddressBalances and GetAddressGroupings (Russell Yanofsky) e7cafab [Refactoring] Mimic ListCoins for sapling notes (random-zebra) 54fa122 [qt] Move some WalletModel functions into CWallet (random-zebra) 494ba64 [test] Add test for getmemoryinfo (random-zebra) 2394083 [Wallet] unset change position when there is no change on exact match (Gregory Sanders) 7d977ac Remove unused C++ code not covered by unit tests (random-zebra) 60bb4da ApproximateBestSubset should take inputs by reference, not value (Ryan Havar) 3633d75 Initialize nRelockTime (Patrick Strateman) 3a599d0 [Refactor] Return safeTx boolean in CheckTXAvailability (random-zebra) f219be9 Add safe flag to listunspent result (NicolasDorier) 0201065 Add COutput::fSafe member for safe handling of unconfirmed outputs (random-zebra) 75c8c6d Disallow copy of CReserveKeys (Gregory Sanders) 8378322 [Refactor] Replace optional reserveKey in PBF with unique pointer (random-zebra) Pull request description: I think these are all the remaining Bitcoin Core v0.15 PRs in the wallet area that we don't have yet, and are useful to us. I've grouped them here since they are all pretty small, simple, and narrow-focused (on the wallet/rpcwallet files). This changeset is based on top of - [x] #2337 as it keeps going with the multiwallet implementation, adding: - RPC endpoint support - `listwallets` RPC - wallet name in `getwalletinfo` RPC - `wallet_multiwallet.py` functional test As in some areas we are much closer to upstream, some of the commits needed adaptations (especially the functional tests). A couple of commits have been extended to include shield-related code. List of upstream PRs backported/adapted: - bitcoin#9906 : Disallow copy constructor CReserveKeys - bitcoin#9830 : Add safe flag to listunspent result - bitcoin#9993 : Initialize nRelockTime - bitcoin#10108 : ApproximateBestSubset should take inputs by reference, not value - bitcoin#10075 : Remove unused C++ code not covered by unit tests - bitcoin#10257 : Add test for getmemoryinfo - bitcoin#10295 : Move some WalletModel functions into CWallet - bitcoin#10500 : Avoid CWalletTx copies in GetAddressBalances and GetAddressGroupings - bitcoin#10522 : Remove unused variables - bitcoin#10816 : Properly forbid -salvagewallet and -zapwallettxes for multi wallet - bitcoin#10849 : Multiwallet: simplest endpoint support - bitcoin#9380 : Separate different uses of minimum fees (only dustRelayFee) - bitcoin#10817 : Redefine Dust and add a discard_rate (only first commit) - bitcoin#10883 : Rename -usewallet to -rpcwallet - bitcoin#10604 : Add listwallets RPC, include wallet name in getwalletinfo + tests - bitcoin#10893 : Avoid running multiwallet.py twice - bitcoin#10870 : Use wallet 0 in rpc console if running with multiple wallets - bitcoin#10931 : Fix misleading "method not found" multiwallet errors - bitcoin#10885 : Reject invalid wallets - bitcoin#11022 : Basic keypool topup - bitcoin#10976 : [MOVEONLY] Move some static functions out of wallet.h/cpp - bitcoin#11310 : Test listwallets RPC - bitcoin#10858 : "errors" to getblockchaininfo + unify "errors" field in get*info RPCs - bitcoin#11492 : Fix leak in CDB constructor - bitcoin#11476 : Avoid opening copied wallet databases simultaneously - bitcoin#11590 : always show help-line of wallet encryption calls - bitcoin#11289 : Add wallet backup text to import* and add* RPCs ACKs for top commit: furszy: re-re-utACK d5526bd. Fuzzbawls: ACK d5526bd Tree-SHA512: 115c4916ee212539b0a1b2cb25783ddf6753f5376de739a084191e874ed8717fff2c7cd9d744e397891f14eaa459ef2f48d675168621ef7316e81758fa6559f2

…d unify "errors" field in get*info RPCs 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 `errors` 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 `errors` 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

fanquake added the RPC/REST/ZMQ label Jul 18, 2017

achow101 force-pushed the getblockchaininfo-errors branch 2 times, most recently from 72cebc4 to 868c4de Compare July 18, 2017 20:17

achow101 force-pushed the getblockchaininfo-errors branch 2 times, most recently from 363af0f to 771d727 Compare August 15, 2017 23:58

achow101 force-pushed the getblockchaininfo-errors branch from 771d727 to 0d39b44 Compare September 19, 2017 02:02

achow101 force-pushed the getblockchaininfo-errors branch from 0d39b44 to fc1af49 Compare September 26, 2017 16:04

achow101 force-pushed the getblockchaininfo-errors branch 2 times, most recently from b10de68 to 0a35219 Compare September 26, 2017 16:13

achow101 force-pushed the getblockchaininfo-errors branch from 0a35219 to 7b05f46 Compare September 26, 2017 17:42

achow101 force-pushed the getblockchaininfo-errors branch from 7b05f46 to 027fcb4 Compare September 26, 2017 21:24

achow101 added 3 commits September 27, 2017 11:27

Add warnings field to getblockchaininfo

f77f0e4

Unify help text for GetWarnings output in get*info RPCs

8502b20

Change getmininginfo errors field to warnings

395cef7

Changes the errors field to warnings. To maintain compatibility, the errors field is deprecated and enabled by starting bitcoind with -deprecatedrpc=getmininginfo

achow101 force-pushed the getblockchaininfo-errors branch from 027fcb4 to 395cef7 Compare September 27, 2017 15:27

jonasschnelli reviewed Sep 28, 2017

View reviewed changes

jonasschnelli mentioned this pull request Sep 28, 2017

Simplify GetWarnings() #11412

Closed

laanwj merged commit 395cef7 into bitcoin:master Sep 28, 2017

maflcko mentioned this pull request Sep 29, 2017

TODO for release notes 0.16.0 #11054

Closed

jnewbery mentioned this pull request Feb 2, 2018

Remove deprecated rpc options #12336

Merged

random-zebra mentioned this pull request Apr 28, 2021

[Wallet] Laggard wallet-related backports from btc 0.15 PIVX-Project/PIVX#2351

Merged

1 task

bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021

[RPC] Add "errors" field to getblockchaininfo and unify "errors" field in get*info RPCs #10858

[RPC] Add "errors" field to getblockchaininfo and unify "errors" field in get*info RPCs #10858

Uh oh!

Conversation

achow101 commented Jul 17, 2017

Uh oh!

laanwj commented Jul 18, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

achow101 commented Jul 18, 2017

Uh oh!

jnewbery commented Sep 1, 2017

Uh oh!

achow101 commented Sep 19, 2017

Uh oh!

jnewbery commented Sep 19, 2017

Uh oh!

achow101 commented Sep 19, 2017

Uh oh!

jnewbery commented Sep 19, 2017

Uh oh!

achow101 commented Sep 19, 2017

Uh oh!

jnewbery commented Sep 19, 2017

Uh oh!

achow101 commented Sep 26, 2017

Uh oh!

jnewbery commented Sep 26, 2017

Uh oh!

achow101 commented Sep 26, 2017

Uh oh!

achow101 commented Sep 27, 2017

Uh oh!

jnewbery commented Sep 27, 2017

Uh oh!

jonasschnelli Sep 28, 2017 • 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!

jonasschnelli commented Sep 28, 2017

Uh oh!

Uh oh!

laanwj commented Jul 18, 2017 •

edited

Loading

jonasschnelli Sep 28, 2017 •

edited

Loading