rpc: Tidy up reporting of buried and ongoing softforks

MarcoFalke commented at 5:57 pm on July 2, 2019: member

This unifies how softforks (e.g. buried, ISM, BIP9, …) are reported. The output of getforkinfo() is now always a dict, where the key is the name of the softfork and the value is a dict with further details.

MarcoFalke force-pushed on Jul 2, 2019

jnewbery commented at 6:22 pm on July 2, 2019: member

utACK fa3d0199a8a03cc7e14b0242d672af0328f201f8

I’ve read the code and it looks good to me.

laanwj commented at 6:22 pm on July 2, 2019: member

How does this fit in with buried softfork changes like #16060?

jnewbery commented at 6:35 pm on July 2, 2019: member

How does this fit in with buried softfork changes like

This is the first commit from #16060, with some changes to the tests and release notes.

MarcoFalke force-pushed on Jul 2, 2019

DrahtBot commented at 7:29 pm on July 2, 2019: member

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#16365 (Log RPC parameters (arguments) if -debug=rpcparams by LarryRuane)
#16229 (Standardise deployment handling by ajtowns)
#16060 (Bury bip9 deployments by jnewbery)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

MarcoFalke renamed this:
~~rpc: Tidy up reporting of buried and ongoing softforks~~
WIP rpc: Tidy up reporting of buried and ongoing softforks
on Jul 2, 2019

MarcoFalke force-pushed on Jul 2, 2019

fanquake added the label RPC/REST/ZMQ on Jul 2, 2019

laanwj commented at 8:42 am on July 3, 2019: member

This is the first commit from #16060, with some changes to the tests and release notes.

Thanks. Yes, probably makes sense to split this off from the burying itself.

MarcoFalke renamed this:
~~WIP rpc: Tidy up reporting of buried and ongoing softforks~~
rpc: Tidy up reporting of buried and ongoing softforks
on Jul 3, 2019

MarcoFalke force-pushed on Jul 3, 2019

in src/rpc/blockchain.cpp:1278 in fae687c6bd outdated

1327             "        \"bit\": xx,              (numeric) the bit (0-28) in the block version field used to signal this softfork (only for \"started\" status)\n"
1328             "        \"startTime\": xx,        (numeric) the minimum median time past of a block at which the bit gains its meaning\n"
1329             "        \"timeout\": xx,          (numeric) the median time past of a block at which the deployment is considered failed if not yet locked in\n"
1330             "        \"since\": xx,            (numeric) height of the first block to which the status applies\n"
1331-            "        \"statistics\": {         (object) numeric statistics about BIP9 signalling for a softfork (only for \"started\" status)\n"
1332+            "        \"statistics\": {         (object) numeric statistics about BIP9 signalling for a softfork\n"

ajtowns commented at 11:42 pm on July 3, 2019:

why drop the “only for started” note?

MarcoFalke commented at 5:55 pm on July 19, 2019:

Restored

in src/rpc/blockchain.cpp:1287 in fae687c6bd outdated

1336             "           \"count\": xx,         (numeric) the number of blocks with the version bit set in the current period \n"
1337             "           \"possible\": xx       (boolean) returns false if there are not enough blocks left in this period to pass activation threshold \n"
1338             "        }\n"
1339+            "      },\n"
1340+            "      \"height\": \"xxxxxx\",       (numeric) height of the first block which the rules are enforced (only for \"buried\" type, or \"bip9\" type with \"locked_in\" or \"active\" status)\n"
1341+            "      \"active\": xx,             (boolean) true if the rules are enforced at the current chain tip\n"

ajtowns commented at 5:40 am on July 4, 2019:

I believe tip->nHeight >= BIPxxHeight tells you if the rules were enforced in the current block, while ThresholdState::ACTIVE == thresholdState tells you if the rules should be enforced in the next block, so I think the value of this field is currently inconsistent. I think jnewbery’s original patch consistently reported whether the rules should be enforced in the next block, by checking tip->nHeight+1 >= BIPxxHeight.

You can test this by changing the BIP66 height for regtest to 1152, and signalling for “csv” between blocks 864 and 1008; you get:

$ bitcoin-cli -regtest getblockchaininfo
{
  "chain": "regtest",
  "blocks": 1151,
  "headers": 1151,
  ...
  "softforks": {
    ...
    "bip66": {
      "type": "buried",
      "active": false,
      "height": 1152
    },
    ...
    "csv": {
      "type": "bip9",
      "bip9": {
        "status": "active",
        "startTime": 0,
        "timeout": 9223372036854775807,
        "since": 1152
      },
      "height": 1152,
      "active": true
    },
    ...
  },
  ...
}

with csv.height == bip66.height, but csv.active != bip66.active.

I think my preference would be to report whether the next block needs to comply with the soft-fork rather than whether the current block did comply with it.

ajtowns commented at 0:22 am on July 5, 2019:

After sleeping on it, I think I’ve changed my mind – it’s probably less confusing to have “active=true” tell you whether the rules were enforced on the current block, and you can use the “height=N” output to tell you whether it needs to be enforced on the next block.

In that case, I think you’d want nHeight >= BIPxxHeight and to use tip->pprev instead of tip for the versionbits state. The versionbits statistics function might need some more tweaking to make sense though.

ajtowns commented at 3:26 am on July 5, 2019:

I’ve pushed an update to https://github.com/ajtowns/bitcoin/commits/201907-getforkinfo that implements the latter in a way I think makes sense, including changes to versionist statistics. Can check the output makes sense with something like (assuming the “csv” softfork activates at block 1152 and you have jshon available):

$ for a in 863 864 865 1006 1007 1008 1009 1010 1150 1151 1152; do echo $a; bitcoin-cli -regtest getforkinfo $(bitcoin-cli -regtest getblockhash $a)  | jshon -e csv; echo ======; done | less

in src/rpc/blockchain.cpp:1190 in fae687c6bd outdated

1253-    rv.pushKV("version", version);
1254-    rv.pushKV("reject", SoftForkMajorityDesc(version, pindex, consensusParams));
1255-    return rv;
1256+    rv.pushKV("type", "buried");
1257+    rv.pushKV("active", tip.nHeight >= height);
1258+    rv.pushKV("height", height);

ajtowns commented at 5:41 am on July 4, 2019:

nit: should be height then active, to match order of bip9 object

ajtowns changes_requested

ajtowns commented at 6:39 am on July 4, 2019: member

I think the “active” field needs fixing one way or the other before merge.

Three other suggestions:

include a "bips": [141, 143, 147] field to make it easy to look up what’s being implemented
change the old names to “heightincb”, “strictder” and “cltv” for consistency (and so you don’t have to remember what bip34 was or which was bip65 and which was bip66)
move this into a “getforkinfo” rpc and out of “getblockchaininfo”; that way getblockchaininfo is just ~14 lines, and the bits that change every block aren’t hidden behind a wall of soft fork info that rarely changes at all

Draft implementation of the above suggestions at https://github.com/ajtowns/bitcoin/commits/201907-getforkinfo if any of them seem worthwhile.

Edited to add: big concept ack!

in src/rpc/blockchain.cpp:1337 in fae687c6bd outdated

1381+    BuriedForkDescPushBack(softforks, "bip34", consensusParams.BIP34Height, *tip);
1382+    BuriedForkDescPushBack(softforks, "bip66", consensusParams.BIP66Height, *tip);
1383+    BuriedForkDescPushBack(softforks, "bip65", consensusParams.BIP65Height, *tip);
1384     for (int pos = Consensus::DEPLOYMENT_CSV; pos != Consensus::MAX_VERSION_BITS_DEPLOYMENTS; ++pos) {
1385-        BIP9SoftForkDescPushBack(bip9_softforks, consensusParams, static_cast<Consensus::DeploymentPos>(pos));
1386+        BIP9SoftForkDescPushBack(softforks, consensusParams, static_cast<Consensus::DeploymentPos>(pos));

jnewbery commented at 12:54 pm on July 4, 2019:

I don’t think reverting this to a for loop makes things any clearer. There are just three bip9 softforks currently, so listing them out explicitly seems clearer to me.

MarcoFalke commented at 8:57 pm on July 8, 2019:

Refactoring changes should (if possible) be in a separate commit from behaviour changes. This can be done in a separate pull request

in src/rpc/blockchain.cpp:1195 in fae687c6bd outdated

1257+    rv.pushKV("active", tip.nHeight >= height);
1258+    rv.pushKV("height", height);
1259+    softforks.pushKV(name, rv);
1260 }
1261 
1262 static UniValue BIP9SoftForkDesc(const Consensus::Params& consensusParams, Consensus::DeploymentPos id)

jnewbery commented at 12:56 pm on July 4, 2019:

Again, I don’t understand why you’ve reverted this from the tidier code in 16060

MarcoFalke commented at 8:57 pm on July 8, 2019:

Refactoring changes should (if possible) be in a separate commit from behaviour changes. This can be done in a separate pull request

jnewbery commented at 12:57 pm on July 4, 2019: member

A couple of comments inline

Incidentally, you’ve force-pushed to this branch 9 times since opening it (7 since I last reviewed), without any additional comments in the PR. Without any explanation for why you’re making those changes, it was a lot of effort to re-review this PR. I don’t see the rationale for reverting some of the previous changes, for example.

fanquake added the label Waiting for author on Jul 6, 2019

in doc/release-notes-16328.md:9 in fae687c6bd outdated

0@@ -0,0 +1,9 @@
1+Low-level changes
2+=================
3+
4+RPC
5+---
6+
7+- The array `softforks` and object `bip9_softforks` in the `getblockchaininfo`
8+  RPC are combined into a single `softforks` object. Refer to the RPC help for
9+  futher details.

practicalswift commented at 9:49 am on July 10, 2019:

Should be “further” :-)

MarcoFalke force-pushed on Jul 19, 2019

MarcoFalke commented at 5:51 pm on July 19, 2019: member

Addressed review from @practicalswift and @ajtowns (excellent stuff, thanks!)

In particular:

Moved it to a new rpc to not clutter exiting rpcs
Made the resulting dict atomic (by acquiring cs_main, to avoid races with incoming blocks)
Made both types consistent (by reintroducing the “off-by-one”)
Renamed “bip9” to “vb”, since BIP 8 is also a version bits deployment mechanism

I left out (from @ajtowns branch):

70f425e84c8d6724099a1e5f63ea309875f34848 because I didn’t like the non-rpc changes. Feel free to submit as a follow up pull request
83709e74672d542ab58b0c70a76a32301236e0d4 because of the additional changes. Feel free to submit as follow up

[rpc] Tidy up reporting of buried and ongoing softforks

This combines reporting of buried (formally ISM) softfork deployments
and BIP9 versionbits softfork deployments into one JSON object

Co-authored-by: Anthony Towns <aj@erisian.com.au>
Co-authored-by: John Newbery <john@johnnewbery.com>

faf41ea6f3

MarcoFalke removed the label Waiting for author on Jul 19, 2019

MarcoFalke force-pushed on Jul 19, 2019

rpc: Renambe bip9 to vb in getforkinfo fab4bd992e

drop VersionBitsTip, explain versionbitscache guard 4a4178b61c

MarcoFalke force-pushed on Jul 19, 2019

ajtowns commented at 11:51 pm on July 21, 2019: member

Not sure why I didn’t think of it earlier, but there’s a much easier way of fixing the off-by-one issue with “active” that doesn’t need changes to versionbits, given we do the “height” calculation anyway:

 0@@ -1189,6 +1189,7 @@ static void BuriedForkDescPushBack(UniValue& softforks, const std::string& name,
 1     rv.pushKV("type", "buried");
 2     rv.pushKV("height", height);
 3     rv.pushKV("active", tip.nHeight + 1 >= height);
 4+    rv.pushKV("enforced", tip.nHeight >= height);
 5 
 6     softforks.pushKV(name, rv);
 7 }
 8@@ -1227,12 +1228,15 @@ static UniValue BIP9SoftForkDesc(const CBlockIndex* pindex, const Consensus::Par
 9     UniValue rv(UniValue::VOBJ);
10     rv.pushKV("type", "vb");
11     rv.pushKV("vb", bip9);
12+    int height = -1;
13     if (ThresholdState::LOCKED_IN == thresholdState) {
14-        rv.pushKV("height", since_height + consensusParams.nMinerConfirmationWindow);
15+        height = since_height + consensusParams.nMinerConfirmationWindow;
16     } else if (ThresholdState::ACTIVE == thresholdState) {
17-        rv.pushKV("height", since_height);
18+        height = since_height;
19     }
20+    if (height >= 0) rv.pushKV("height", height);
21     rv.pushKV("active", ThresholdState::ACTIVE == thresholdState);
22+    rv.pushKV("enforced", (height >= 0 && pindex->nHeight >= height));
23 
24     return rv;
25 }

(I added a differently named field to make sure "active": True matches up with "vb": { "status": "active", .. }, since bumping the info in the “vb” field really needs changes to versionbits.cpp to make much sense.

Also, you’ve got a typo in the commit message, “Renambe bip9..”.

MarcoFalke closed this on Jul 23, 2019

MarcoFalke deleted the branch on Jul 23, 2019

DrahtBot locked this on Dec 16, 2021

rpc: Tidy up reporting of buried and ongoing softforks #16328

Conflicts