Details

Reviewers

deadalnix
jasonbcox
Fabien
nakihito

Group Reviewers

Restricted Project

Commits

rSTAGING785fd9d744f9: [rpc] deprecate ancient softforks' information from getblockchaininfo
rABC785fd9d744f9: [rpc] deprecate ancient softforks' information from getblockchaininfo

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

Branch

no-rpc-softforks

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 6405
Build 10857: Bitcoin ABC Buildbot (legacy)
Build 10856: arc lint + arc unit

Event Timeline

markblundeberg created this revision.Jun 17 2019, 20:53

Herald added a reviewer: Restricted Project. · View Herald TranscriptJun 17 2019, 20:53

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing started.

markblundeberg edited the test plan for this revision. (Show Details)Jun 17 2019, 20:54

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing passed.

Harbormaster completed remote builds in B6385: Diff 9502.Jun 17 2019, 21:03

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.

change to deprecation instead of removal

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing started.

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing passed.

Harbormaster completed remote builds in B6399: Diff 9530.Jun 19 2019, 05:30

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

update per discussion

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing started.

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing passed.

Harbormaster completed remote builds in B6405: Diff 9537.Jun 19 2019, 15:56

Fabien accepted this revision.Jun 19 2019, 16:41

Just a little nit, but otherwise good.

src/rpc/blockchain.cpp
1301	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

markblundeberg added inline comments.Jun 19 2019, 21:16

src/rpc/blockchain.cpp
1301	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.

nakihito added inline comments.Jun 19 2019, 21:35

src/rpc/blockchain.cpp
1301	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.