rpc: Tidy up reporting of buried and ongoing softforks

[maflcko]

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.

[jnewbery]

utACK fa3d019

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

[laanwj]

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

[jnewbery]

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.

[DrahtBot]

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.

[laanwj]

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.

[ajtowns]

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!

[jnewbery]

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.

[maflcko]

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):

• 70f425e because I didn't like the non-rpc changes. Feel free to submit as a follow up pull request
• 83709e7 because of the additional changes. Feel free to submit as follow up

[ajtowns]

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:

@@ -1189,6 +1189,7 @@ static void BuriedForkDescPushBack(UniValue& softforks, const std::string& name,
     rv.pushKV("type", "buried");
     rv.pushKV("height", height);
     rv.pushKV("active", tip.nHeight + 1 >= height);
+    rv.pushKV("enforced", tip.nHeight >= height);
 
     softforks.pushKV(name, rv);
 }
@@ -1227,12 +1228,15 @@ static UniValue BIP9SoftForkDesc(const CBlockIndex* pindex, const Consensus::Par
     UniValue rv(UniValue::VOBJ);
     rv.pushKV("type", "vb");
     rv.pushKV("vb", bip9);
+    int height = -1;
     if (ThresholdState::LOCKED_IN == thresholdState) {
-        rv.pushKV("height", since_height + consensusParams.nMinerConfirmationWindow);
+        height = since_height + consensusParams.nMinerConfirmationWindow;
     } else if (ThresholdState::ACTIVE == thresholdState) {
-        rv.pushKV("height", since_height);
+        height = since_height;
     }
+    if (height >= 0) rv.pushKV("height", height);
     rv.pushKV("active", ThresholdState::ACTIVE == thresholdState);
+    rv.pushKV("enforced", (height >= 0 && pindex->nHeight >= height));
 
     return rv;
 }

(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..".