Page MenuHomePhabricator

[rpc] deprecate ancient softforks' information from getblockchaininfo
ClosedPublic

Authored by markblundeberg on Jun 17 2019, 20:53.

Details

Summary

I don't think anyone cares about this, and in fact it is a bit annoying
since it fills a typical 80x25 terminal with essentialy useless information
that causes the important stuff to scroll out of view.

Test Plan

make check
test_runner.py --extended
read bitcoin-cli help getblockchaininfo
try with bitcoind -deprecatedrpc=getblockchaininfo

Diff Detail

Repository
rABC Bitcoin ABC
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

With patch:

{
  "chain": "test",
  "blocks": 1309568,
  "headers": 1303313,
  "bestblockhash": "0000000000000c97db875750712183dc3d608d225e275ce992a647ba084ea848",
  "difficulty": 842811.5393743159,
  "mediantime": 1560802822,
  "verificationprogress": 0.9999993508198258,
  "initialblockdownload": false,
  "chainwork": "00000000000000000000000000000000000000000000004f5d4024d8d2d6fd56",
  "size_on_disk": 26073090536,
  "pruned": false,
  "warnings": ""
}

Without patch:

{
  "chain": "test",
  "blocks": 1309568,
  "headers": 1303313,
  "bestblockhash": "0000000000000c97db875750712183dc3d608d225e275ce992a647ba084ea848",
  "difficulty": 842811.5393743159,
  "mediantime": 1560802822,
  "verificationprogress": 0.9999949466916916,
  "initialblockdownload": false,
  "chainwork": "00000000000000000000000000000000000000000000004f5d4024d8d2d6fd56",
  "size_on_disk": 26073090536,
  "pruned": false,
  "softforks": [
    {
      "id": "bip34",
      "version": 2,
      "reject": {
        "status": true
      }
    },
    {
      "id": "bip66",
      "version": 3,
      "reject": {
        "status": true
      }
    },
    {
      "id": "bip65",
      "version": 4,
      "reject": {
        "status": true
      }
    },
    {
      "id": "csv",
      "version": 5,
      "reject": {
        "status": true
      }
    }
  ],
  "warnings": ""
}

While there probably isn't very much practical use for the information, I'd image it would be better to deprecate that functionality rather than just outright remove it.

nakihito requested changes to this revision.Jun 17 2019, 23:22
This revision now requires changes to proceed.Jun 17 2019, 23:22

@nakihito hmmm, would that -deprecatedrpc thing be appropriate here (sort of like what happened with validateaddress in the recent release)? Or you mean it should be marked deprecated in helptext for some time before being taken out?

I definitely would want a second opinion here, but I think I would just deprecate the soft fork functionality using -deprecatedrpc and IsDeprecateRPCEnabled. D2846 is an example where I deprecated the functionality of part of an RPC including adding a test case. You should be able to do something similar here.

Imo use of -deprecatedrpc is a good idea here.
Make sure to mention this option in the release notes.

OK, cool! I think that's a good way to go as well.

markblundeberg retitled this revision from [rpc] remove ancient softforks' information from getblockchaininfo to [rpc] deprecate ancient softforks' information from getblockchaininfo.
markblundeberg edited the test plan for this revision. (Show Details)

change to deprecation instead of removal

Fabien requested changes to this revision.Jun 19 2019, 10:53

Please add a test in rpc_deprecated.py to ensure the softforks information is returned when the deprecatedrpc=getblockchaininfo flag is set.

doc/release-notes.md
7 ↗(On Diff #9530)

Please add an in info on how to recover the old behavior, something like:
To keep the 'softforks' information in the rpc output, start bitcoind with the '-deprecatedrpc=getblockchaininfo' option.
Adding this tip in the release notes has been requested by (at least one) users in the past.

This revision now requires changes to proceed.Jun 19 2019, 10:53
nakihito requested changes to this revision.Jun 19 2019, 17:12

Just a little nit, but otherwise good.

src/rpc/blockchain.cpp
1301 ↗(On Diff #9537)

I think this part should be shown when deprecated. The deprecated behavior should match the current behavior as close as possible.

This revision now requires changes to proceed.Jun 19 2019, 17:12
src/rpc/blockchain.cpp
1301 ↗(On Diff #9537)

Hmm yeah I was considering that, but it doesn't quite match what happened with validateaddress, just as an example. OTOH to give a contrastic example though, getmininginfo just shows the deprecated field all the time in the help.

I can't find an example where the help text changes based on IsDeprecatedRPCEnabled.

src/rpc/blockchain.cpp
1301 ↗(On Diff #9537)

Ideally we should definitely have a uniform format for this i.e. always show, only shows when -deprecatedrpc is enabled, or always shows but notes the text that is deprecated. It seems Core tends to follow the last option, so I would just change line 1290 to include DEPRECATED. like D2846.

restore help text, per discussion

This revision is now accepted and ready to land.Jun 19 2019, 22:28

(final rebase was necessary for release notes)