Add getdeploymentinfo RPC

ajtowns commented at 1:29 am on November 14, 2021: member

The aim of this PR is to improve the ability to monitor soft fork status. It first moves the softfork section from getblockchaininfo into a new RPC named getdeploymentinfo, which is then also able to query the status of forks at an arbitrary block rather than only at the tip. In addition, bip9 status is changed to indicate the status of the given block, rather than just for the next block, and an additional field is included to indicate whether each block in the signalling period signaled.

fanquake added the label RPC/REST/ZMQ on Nov 14, 2021

ajtowns commented at 1:39 am on November 14, 2021: member

Example output:

 0$ ./bitcoin-cli getdeploymentinfo 000000000000000000013712fc242ee6dd28476d0e9c931c75f83e6974c6bccc | jq .hash,.height,.deployments.taproot
 1"000000000000000000013712fc242ee6dd28476d0e9c931c75f83e6974c6bccc"
 2709631
 3{
 4  "type": "bip9",
 5  "height": 709632,
 6  "active": true,
 7  "bip9": {
 8    "bit": 2,
 9    "start_time": 1619222400,
10    "timeout": 1628640000,
11    "since": 687456,
12    "min_activation_height": 709632,
13    "status": "locked_in",
14    "status-next": "active",
15    "statistics": {
16      "period": 2016,
17      "elapsed": 2016,
18      "count": 1891
19    },
20    "signalling": "#########-######-######################################-####################-###################################-##################################################-##################-############################################-#######-#####################-#########-#######################################-########-#######-##################-######-#################-####################-###########-#########################-##########-######################################-#####-###########-####-###############################-##-###-#############-###-########-###########-#######-#-##################--########################-############-#####-###########-###-################-####################################-###-#####-###############-#######################-######################-###########################################################-#-#-#######################-######-########################-##########-####################-####################-################--##-#######################-#########-############-#####-#######################-#############-#####################-##################-##########-####-####-############-#######-#########-##################################################-#############################-###############-######################-#####-###########################-##-##-######-########################-#########-##-##################-######################-########################################-####-########################-#########################################################################-####-########-#####-#######################-##########-##########-########################-#-######################################-#############-####-#####-##############-#-############-#############-###-######-##################-############################-########-#########-########-#######-############################################-#-#######################################-#-###########-########-##########-############-##########-#########"
21  }
22}
23$ ./bitcoin-cli getdeploymentinfo $(./bitcoin-cli getblockhash 683423) | jq '.deployments.taproot'
24{
25  "type": "bip9",
26  "active": false,
27  "bip9": {
28    "bit": 2,
29    "start_time": 1619222400,
30    "timeout": 1628640000,
31    "since": 681408,
32    "min_activation_height": 709632,
33    "status": "started",
34    "status-next": "started",
35    "statistics": {
36      "period": 2016,
37      "elapsed": 2016,
38      "count": 823,
39      "threshold": 1815,
40      "possible": false
41    },
42    "signalling": 
43        "--------------------------------------------------#------------------------------------------------------#------------------#----------#------##
44         ----#-----#---#--#-----#-------#----#------#---###----##----#---#--------#---#-#--------#----##-#----#------------------###--####-----------#---
45         #-#----##-------#----#-----#-#-#--##---#-#---##-#-#-##---#---------#-----#---------------##---#---#---##--#----#--#---#--##-#--#----#------#-#--
46         ##-##-----#---#--#-##--#-#####---#-#--##---#-#-#----#-----#----#---#-#-#-##--###----#-#----####--###--#-----#-####--####--##-#-##----#----#----#
47         #-#--#####---#-#-#--##-#--###-###-##--#-#---###-#----#-###-#----####--#---############---#--#-####-----####------#-#--###--##-#-#---########-#--
48         #-#-#--###--###-----#--#--##-----#-###-###-####----#-#--#-##-#----#---#-######----#####-#---#---###-##--###-##----##-#####-#--#-###-#----###-##-
49         ##--##---#-#-#-##-##-#-#--#--#-##--#-##-#-#####--#-#---#########----##-###-#-#-#-----###--#-##-#---##--#-###-----##-----#-##--###--#-##-###---#-
50         ###-###-----##-#---#------#-###-##-#-##---#--#---#--#-##--###-##-##---#-#-#----#--#--#-#-##--#--#-###--###--#--#---#####----#--#---#####---#-##-
51         ----###--##---#--##-#---##-#-------#---#--#-#---##-##---###----#-#-#---#-#---##---#-#-##--#-###--##--#------#-###--#-#---##----#####-#-----#-###
52         #-#------###-##-###-------###----#----##-#--#--##----####-##--#-#-#-#-##--##----#--##-#-##--#--#-#-###-###---------####-#####--#--#--##-------##
53         ---##--#--#-##-##---##-###-#----#----##-#-#---###-#--##--#---#-----#-#-#-----###-##-------#-#-####--##-####--####-#---#-#--##-#---#-##------#-#-
54         -#--#-####-#------#-#-###-----##--##-##-#####-##-####--#----#---#---#-##-###-###-######------###--##---#----####-#------###-###-#------#####-##-
55         ###-#-###-#--#--###--#####---##-#-#-#-#---#######-#-#---#-#----###-##----##-#-#-#-#-##-#-#--###-#-#-#####---#--#--##-----#----#-#-#----#-##-----
56         -##---##--###---###--#-##--#--###-#-##-##--##-----#-#-##-#-#--#-#-#--#-##-##-##-#--######--#--##-#-#-#---##-##---#---###-#--#----##--#-#-#-#-##-"
57  }
58}

(signalling results split into sections of 144 blocks by hand so it’s easier to see on github; 683423 is the last block in taproot’s first signalling period)

ajtowns force-pushed on Nov 14, 2021

DrahtBot commented at 3:05 am on November 14, 2021: 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:

#24032 (Refactor vDeployments setup to avoid uninitialized variables by ajtowns)
#22016 (rpc: add period_start to version bits statistics by Sjors)
#21702 (Implement BIP-119 Validation (CheckTemplateVerify) by JeremyRubin)

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.

ajtowns force-pushed on Nov 14, 2021

MarcoFalke commented at 9:14 am on November 14, 2021: member

My first thought was that getchaintips already reports all chain “forks”, so this won’t be needed. Maybe this could be named deploymentinfo or so to avoid confusion with cutlery?

ajtowns force-pushed on Nov 14, 2021

ajtowns renamed this:
~~Add getforkinfo RPC~~
Add getdeploymentinfo RPC
on Nov 14, 2021

ajtowns commented at 1:11 pm on November 14, 2021: member

Changed from getforkinfo to getdeploymentinfo, since it seemed like a knife idea.

benthecarman commented at 11:06 pm on November 14, 2021: contributor

Concept ACK

this is really cool!

brunoerg approved

brunoerg commented at 11:04 am on November 15, 2021: member

tACK 684821c52b5894e0407bcbd853fa3e8323049e83 (MacOS 11.3)

 0➜  bitcoin git:(ajtowns) ✗ ./src/bitcoin-cli getdeploymentinfo
 1{
 2  "*": {
 3    "hash": "00000000000001f0c73d20c4843c1eac8658b5f49e2d80032ff1feaeb543fe35",
 4    "height": 207120
 5  },
 6  "bip34": {
 7    "type": "buried",
 8    "active": false,
 9    "height": 227931
10  },
11  "bip66": {
12    "type": "buried",
13    "active": false,
14    "height": 363725
15  },
16  "bip65": {
17    "type": "buried",
18    "active": false,
19    "height": 388381
20  },
21  "csv": {
22    "type": "buried",
23    "active": false,
24    "height": 419328
25  },
26  "segwit": {
27    "type": "buried",
28    "active": false,
29    "height": 481824
30  },
31  "taproot": {
32    "type": "bip9",
33    "active": false,
34    "bip9": {
35      "start_time": 1619222400,
36      "timeout": 1628640000,
37      "since": 0,
38      "min_activation_height": 709632,
39      "status": "defined",
40      "status-next": "defined"
41    }
42  }
43}
44➜  bitcoin git:(ajtowns) ✗ ./src/bitcoin-cli getdeploymentinfo | jq '."*", .taproot'
45{
46  "hash": "0000000000000228e53941db17bc6eb44079f4e1677f39eb8afc73335842da68",
47  "height": 210280
48}
49{
50  "type": "bip9",
51  "active": false,
52  "bip9": {
53    "start_time": 1619222400,
54    "timeout": 1628640000,
55    "since": 0,
56    "min_activation_height": 709632,
57    "status": "defined",
58    "status-next": "defined"
59  }
60}

Functional tests:

w0xlt approved

w0xlt commented at 7:02 pm on November 18, 2021: contributor

tACK 684821c on Ubuntu 21.10.

luke-jr changes_requested

luke-jr commented at 7:45 pm on November 30, 2021: member

Should remain in getblockchaininfo, at least as deprecated, for now

ajtowns force-pushed on Dec 2, 2021

ajtowns commented at 5:31 am on December 2, 2021: member

Updated. -deprecatedrpc=softforks will retain the softforks info in getblockchaininfo. Also tweaked the getdeploymentinfo output.

laanwj commented at 5:44 pm on December 8, 2021: member

Concept ACK. I was first skeptical about moving this to a new RPC, but being able to pass in a block hash to evaluate the status as of an arbitrary block is a great idea.

in src/rpc/blockchain.cpp:1592 in 797b8533f7 outdated

1597+                                {RPCResult::Type::NUM, "height", "block height"},
1598+                            }},
1599+                            {RPCResult::Type::OBJ, "xxxx", "name of the softfork",
1600+                            {
1601+                                {RPCResult::Type::STR, "type", "one of \"buried\", \"bip9\""},
1602+                                {RPCResult::Type::NUM, "height", /* optional */ true, "height of the first block which the rules are or will be enforced (only for \"buried\" type, or \"bip9\" type with \"active\" status)"},

fanquake commented at 9:06 am on December 9, 2021:

Please use this syntax so arguments can be automatically checked by clang-tidy. This is what we are migrating towards. i.e #23545.

0                                {RPCResult::Type::NUM, "height", /*optional=*/true, "height of the first block which the rules are or will be enforced (only for \"buried\" type, or \"bip9\" type with \"active\" status)"},

ajtowns commented at 12:33 pm on December 9, 2021:

Thanks, updated

DrahtBot added the label Needs rebase on Dec 9, 2021

ajtowns force-pushed on Dec 9, 2021

ajtowns commented at 12:33 pm on December 9, 2021: member

Rebased past #23703 and tweaked the commit descriptions slightly.

For testing on mainnet, the following little shell snippet shows some interesting block heights:

0EARLY="0 1"
1BURIED="227930 363724 388380 419327 481823"
2TAPROOT="681406 681407 681408 681409 683423 685439 687455 689471 691487 693503 695519 697535 699551 701567 703583 705599 707615 709631 711647"
3for a in $EARLY $BURIED $TAPROOT; do ./bitcoin-cli getdeploymentinfo $(./bitcoin-cli getblockhash $a); done | less

Note that "active": true indicates a soft fork is being applied as of the next block, hence the blocks above are 1 below the actual activation heights.

DrahtBot removed the label Needs rebase on Dec 9, 2021

laanwj added the label Needs release note on Dec 9, 2021

laanwj commented at 5:25 pm on December 9, 2021: member

Needs release note wrt deprecation.

in src/versionbits.h:70 in 4662cf3a81 outdated

63@@ -64,8 +64,10 @@ class AbstractThresholdConditionChecker {
64     virtual int Threshold(const Consensus::Params& params) const =0;
65 
66 public:
67-    /** Returns the numerical statistics of an in-progress BIP9 softfork in the current period */
68-    BIP9Stats GetStateStatisticsFor(const CBlockIndex* pindex, const Consensus::Params& params) const;
69+    /** Returns the numerical statistics of an in-progress BIP9 softfork in the period including pindex
70+     * If provided, signals is set to true/false based on whether each block in the period signalled
71+     */
72+    BIP9Stats GetStateStatisticsFor(const CBlockIndex* pindex, const Consensus::Params& params, std::vector<bool>* signals = nullptr) const;

laanwj commented at 5:34 pm on December 9, 2021:

signals is not a particularly good name for this parameter, imo, i had no idea what was meant (or what the index is) at first, maybe signallingBlocks?

ajtowns commented at 8:29 am on December 13, 2021:

Changed to signalling_blocks

ajtowns force-pushed on Dec 13, 2021

ajtowns commented at 8:29 am on December 13, 2021: member

Added release note

in src/rpc/blockchain.cpp:1621 in a4749f9154 outdated

1588+static RPCHelpMan getdeploymentinfo()
1589+{
1590+    return RPCHelpMan{"getdeploymentinfo",
1591+        "Returns an object containing various state info regarding soft-forks.\n",
1592+        {},
1593+        RPCResult{

Sjors commented at 6:15 am on December 16, 2021:

a4749f9154a579c8da9d6353587fe83c72a8fc31: you could avoid the duplication here with a helper function, see e.g. bumpfee_helper in wallet/rpc/spend.cpp

Otherwise, maybe reduce indentation.

in src/rpc/blockchain.cpp:1654 in a4749f9154 outdated

1624+            const CBlockIndex* tip = active_chainstate.m_chain.Tip();
1625+            CHECK_NONFATAL(tip);
1626+            const Consensus::Params& consensusParams = Params().GetConsensus();
1627+
1628+            UniValue deploymentinfo(UniValue::VOBJ);
1629+            deploymentinfo.pushKV("deployments", DeploymentInfo(tip, consensusParams));

Sjors commented at 6:18 am on December 16, 2021:

Nesting everything under deployments seems useless.

ajtowns commented at 9:02 am on December 17, 2021:

It separates out height and hash which are introduced in the following commit.

Sjors commented at 6:51 am on December 16, 2021: member

Concept ACK

8d2d1b29cd6cd5799c47c0debfe63e2eddc6a347 is a breaking change. For my own use case in ForkMonitor it’s fine, but if someone relies on the status field before putting something in the mempool right in time for the first block of a new softfork, they’d miss it. But that’s not a problem anytime soon… It would be good to add a test case for a block right before activation (where active should be true, state should be locked_in and state_next should be active).

ajtowns commented at 10:45 am on December 17, 2021: member

8d2d1b2 is a breaking change. For my own use case in ForkMonitor it’s fine, but if someone relies on the status field before putting something in the mempool right in time for the first block of a new softfork, they’d miss it.

Using active is far better than using status for that case, since it is valid for non-bip9 deployments as well. Also there are no pending deployments for which anyone will be mining such a block (so you would need to either be using regtest with non-default config, or trying to do a massive reorg of mainnet or testnet).

ajtowns commented at 4:13 am on January 13, 2022: member

@Sjors @laanwj @luke-jr are your concerns adequately addressed?

Sjors commented at 12:24 pm on January 13, 2022: member

It would be good to add a test case for a block right before activation

This test would be nice. None of my comments are blocking.

ajtowns commented at 3:18 pm on January 13, 2022: member

@Sjors added such a test

Sjors commented at 5:31 pm on January 13, 2022: member

I improved the RPC help a bit in https://github.com/Sjors/bitcoin/commit/af1070dced9a39658799e93c9842d7aab0eb40c4:

less excessive indentation
get rid of "*" : { structure (hash and height are same level as deployments)
nest forks inside deployments
clarify that hash and height use the tip by default

Before:

0Result:
1{                                     (json object)
2  "*" : {                             (json object)
3    "hash" : "str",                   (string) block hash
4    "height" : n                      (numeric) block height
5  },
6  "xxxx" : {                          (json object) name of the softfork
7    "type" : "str",                   (string) one of "buried", "bip9"

After:

0Result:
1{                                       (json object)
2  "hash" : "str",                       (string) requested block hash (or tip)
3  "height" : n,                         (numeric) requested block height (or tip)
4  "deployments" : {                     (json object) status of softforks
5    "xxxx" : {                          (json object) name of the softfork
6      "type" : "str",                   (string) one of "buried", "bip9"

in doc/release-notes-23508.md:8 in 517356dafd outdated

0@@ -0,0 +1,8 @@
1+Updated RPCs
2+------------
3+
4+Information on soft fork status has been moved from `getblockchaininfo` to
5+`getdeploymentinfo` which allows querying soft fork status at any block,
6+rather than just at the chain tip. Inclusion of soft fork status in
7+`getblockchaininfo` can currently be restored using the configuration
8+`-deprecatedrpc=softforks`, but this will be removed in a future release.

MarcoFalke commented at 5:35 pm on January 13, 2022:

0`-deprecatedrpc=softforks`, but this will be removed in a future release. (#23508)

nit

in doc/release-notes-23508.md:4 in 517356dafd outdated

0@@ -0,0 +1,8 @@
1+Updated RPCs
2+------------
3+
4+Information on soft fork status has been moved from `getblockchaininfo` to

MarcoFalke commented at 5:35 pm on January 13, 2022:

0- Information on soft fork status has been moved from `getblockchaininfo` to

nit: items in the notes are lists

in src/rpc/blockchain.cpp:1604 in 517356dafd outdated

1609 }
1610 
1611+static RPCHelpMan getdeploymentinfo()
1612+{
1613+    return RPCHelpMan{"getdeploymentinfo",
1614+        "Returns an object containing various state info regarding soft-forks.\n",

MarcoFalke commented at 5:36 pm on January 13, 2022:

0        "Returns an object containing various state info regarding soft-forks.",

nit: not needed

MarcoFalke commented at 5:37 pm on January 13, 2022: member

left some stupid nits (feel free to ignore)

laanwj added this to the "Blockers" column in a project

ajtowns force-pushed on Jan 13, 2022

ajtowns commented at 11:39 pm on January 13, 2022: member

@Sjors Thanks, included (combined with the nit from @MarcoFalke). @MarcoFalke, de-nitted.

Sjors commented at 12:30 pm on January 14, 2022: member

@ajtowns thanks. Unless you’re waiting for additional feedback, I suggest squashing the last two commits in to earlier ones. That makes reviewing the individual commits more straight-forward.

ajtowns force-pushed on Jan 14, 2022

rpc: move softfork info from getblockchaininfo to getdeploymentinfo fd826130a0

rpc: getdeploymentinfo: allow specifying a blockhash other than tip 7f15c1841b

rpc: getdeploymentinfo: change stats to always refer to current period

On a period boundary, getdeploymentinfo (and previously getblockchaininfo)
would report the status and statistics for the next block rather than
the current block. Change this to always report the status/statistics
of the current block, but add status-next to report the status for the
next block.

a7469bcd35

rpc: getdeploymentinfo: include block hash/height 376c0c6dae

rpc: getdeploymentinfo: include signalling info 240cad09ba

Release notes for getdeploymentinfo rpc a380922891

ajtowns force-pushed on Jan 14, 2022

ajtowns commented at 6:46 pm on January 14, 2022: member

@Sjors okay, done that, and also deduplicated the help text between getblockchaininfo and getdeploymentinfo.

in src/rpc/blockchain.cpp:1610 in 7f15c1841b outdated

1606@@ -1605,8 +1607,18 @@ static RPCHelpMan getdeploymentinfo()
1607             LOCK(cs_main);
1608             CChainState& active_chainstate = chainman.ActiveChainstate();
1609 
1610-            const CBlockIndex* tip = active_chainstate.m_chain.Tip();
1611-            CHECK_NONFATAL(tip);
1612+            const CBlockIndex* tip;

Sjors commented at 8:41 pm on January 14, 2022:

7f15c1841b98de6931a7ac68e16635a05d3e96cf: this variable should probably be called block. I suggest also renaming the tip parameter of DeploymentInfo( and the active_chain_tip parameter of SoftForkDescPushBack to block

ajtowns commented at 6:52 pm on January 20, 2022:

Agree, but leaving this for later

Sjors approved

Sjors commented at 8:53 pm on January 14, 2022: member

tACK a3809228917b8f750090c8bfec8e283391dbb524

Small steps to making this stuff less confusing… (see also https://github.com/btcsuite/btcd/pull/1700#issuecomment-1013128081)

in src/versionbits.cpp:110 in 240cad09ba outdated

107@@ -108,18 +108,26 @@ BIP9Stats AbstractThresholdConditionChecker::GetStateStatisticsFor(const CBlockI
108     if (pindex == nullptr) return stats;
109 
110     // Find beginning of period

fjahr commented at 9:56 pm on January 15, 2022:

This comment doesn’t seem to fit the code anymore

ajtowns commented at 7:33 am on January 28, 2022:

Addressed in #24187

in src/rpc/blockchain.cpp:1459 in fd826130a0 outdated

1454@@ -1455,8 +1455,16 @@ static void SoftForkDescPushBack(const CBlockIndex* active_chain_tip, UniValue&
1455     softforks.pushKV(DeploymentName(id), rv);
1456 }
1457 
1458+namespace {
1459+/* TODO: when -dprecatedrpc=softforks is removed, drop these */

fjahr commented at 9:59 pm on January 15, 2022:

nit: typo in deprecated

ajtowns commented at 7:33 am on January 28, 2022:

Fixed in #24187

MarcoFalke removed the label Needs release note on Jan 20, 2022

in src/rpc/blockchain.cpp:1444 in a7469bcd35 outdated

1450-    int64_t since_height = g_versionbitscache.StateSinceHeight(active_chain_tip, consensusParams, id);
1451-    bip9.pushKV("since", since_height);
1452+    bip9.pushKV("min_activation_height", consensusParams.vDeployments[id].min_activation_height);
1453+
1454+    // BIP9 status
1455+    bip9.pushKV("status", get_state_name(current_state));

MarcoFalke commented at 9:01 pm on January 20, 2022:

Looks like a behavior change, so might be worth to mention in the release notes if you retouch.

MarcoFalke commented at 5:49 pm on January 27, 2022:

If you don’t retouch, I think this should be fixed up in a follow-up

ajtowns commented at 7:33 am on January 28, 2022:

Followed up in #24187

fjahr commented at 0:32 am on January 21, 2022: member

tACK a3809228917b8f750090c8bfec8e283391dbb524

Reviewed code and tested many historical statistics around the Taproot deployment.

in src/rpc/blockchain.cpp:2810 in a380922891

2806@@ -2716,6 +2807,7 @@ static const CRPCCommand commands[] =
2807     { "blockchain",         &getblockheader,                     },
2808     { "blockchain",         &getchaintips,                       },
2809     { "blockchain",         &getdifficulty,                      },
2810+    { "blockchain",         &getdeploymentinfo,                  },

jonatack commented at 4:43 pm on January 27, 2022:

fd82613 this line should perhaps be before getdifficulty, not sure, feel free to ignore

ajtowns commented at 7:04 am on January 28, 2022:

These aren’t in alphabetical order, and I don’t think it quite fits in with any of the other categories of functions (info about the tip, mempool stuff, stats, queries about a particular block; and the functions aren’t really grouped by category anyway), so not seeing much benefit to moving it.

in src/rpc/blockchain.cpp:1635 in a380922891

1634+        RPCExamples{ HelpExampleCli("getdeploymentinfo", "") + HelpExampleRpc("getdeploymentinfo", "") },
1635+        [&](const RPCHelpMan& self, const JSONRPCRequest& request) -> UniValue
1636+        {
1637+            ChainstateManager& chainman = EnsureAnyChainman(request.context);
1638+            LOCK(cs_main);
1639+            CChainState& active_chainstate = chainman.ActiveChainstate();

jonatack commented at 5:05 pm on January 27, 2022:

fd826130a0a4e67fdc26f8064f4ecb4ff79b3333

0-            ChainstateManager& chainman = EnsureAnyChainman(request.context);
1-            LOCK(cs_main);
2-            CChainState& active_chainstate = chainman.ActiveChainstate();
3+            const ChainstateManager& chainman = EnsureAnyChainman(request.context);
4+            LOCK(::cs_main);
5+            const CChainState& active_chainstate = chainman.ActiveChainstate();

ajtowns commented at 7:34 am on January 28, 2022:

Added in #24187

in src/rpc/blockchain.cpp:1642 in a380922891

1641+            const CBlockIndex* tip;
1642+            if (request.params[0].isNull()) {
1643+                tip = active_chainstate.m_chain.Tip();
1644+                CHECK_NONFATAL(tip);
1645+            } else {
1646+                uint256 hash(ParseHashV(request.params[0], "blockhash"));

jonatack commented at 5:09 pm on January 27, 2022:

7f15c184

0                const uint256& hash{ParseHashV(request.params[0], "blockhash")};

ajtowns commented at 7:34 am on January 28, 2022:

Added in #24187

in src/rpc/blockchain.cpp:1619 in a380922891

1618+static RPCHelpMan getdeploymentinfo()
1619+{
1620+    return RPCHelpMan{"getdeploymentinfo",
1621+        "Returns an object containing various state info regarding soft-forks.",
1622+        {
1623+            {"blockhash", RPCArg::Type::STR_HEX, RPCArg::Default{"chain tip"}, "The block hash at which to query fork state"},

jonatack commented at 5:12 pm on January 27, 2022:

7f15c1841b98de6931a7ac68e16635a05d3e96cf current naming style for new rpc arguments, and a default description that (maybe) looks a little less like “chain tip” could be a value

0Arguments:
11. blockhash    (string, optional, default="chain tip") The block hash at which to query fork state

0            {"block_hash", RPCArg::Type::STR_HEX, RPCArg::Default{"value of chain tip"}, "The block hash at which to query fork state"},

It would be good to have tests for invoking getdeploymentinfo with a valid and an invalid block hash.

Sjors commented at 5:53 pm on January 27, 2022:

This code is reused in the deprecated getblockchaininfo part, so let’s not rename these things. Except for new items.

jonatack commented at 6:27 pm on January 27, 2022:

Oh that’s true; the functionality is nicely reused. Correcting myself, the argument is only in the new RPC, so it could follow current naming style.

ajtowns commented at 6:42 am on January 28, 2022:

Ugh, why are we introducing new naming styles that are incompatible with the existing names, especially where they’re part of an API? :vomiting_face:

in doc/release-notes-23508.md:5 in a380922891

0@@ -0,0 +1,9 @@
1+Updated RPCs
2+------------
3+
4+- Information on soft fork status has been moved from `getblockchaininfo`
5+  to `getdeploymentinfo` which allows querying soft fork status at any

jonatack commented at 5:24 pm on January 27, 2022:

a3809228

0  to a new `getdeploymentinfo` RPC, which allows querying soft fork status at any

ajtowns commented at 7:34 am on January 28, 2022:

Added in #24187

in src/rpc/blockchain.cpp:1617 in a380922891

1616-    return obj;
1617-},
1618+static RPCHelpMan getdeploymentinfo()
1619+{
1620+    return RPCHelpMan{"getdeploymentinfo",
1621+        "Returns an object containing various state info regarding soft-forks.",

jonatack commented at 5:26 pm on January 27, 2022:

fd82613 It looks like you are writing “softfork” in the RPC documentation, and “soft fork” in the release note, so maybe standardize (on soft fork) throughout.

0        "Returns an object containing various state info regarding soft forks.",

MarcoFalke commented at 6:01 pm on January 27, 2022:

buried deployments aren’t soft forks, so it might be better to call this “deployments”

ajtowns commented at 7:35 am on January 28, 2022:

Switched from soft[ -]*forks in #24187. Note that rpc/miner.cpp calls them “softforks” in its RPC docs…

MarcoFalke commented at 7:44 am on January 28, 2022:

Yeah, I am not suggesting to change existing fields, but for new ones we can pick a good name from the beginning.

laanwj commented at 5:39 pm on January 27, 2022: member

Code review and lightly tested ACK a3809228917b8f750090c8bfec8e283391dbb524

in src/rpc/blockchain.cpp:1587 in a380922891

1583+        {RPCResult::Type::NUM_TIME, "start_time", "the minimum median time past of a block at which the bit gains its meaning"},
1584+        {RPCResult::Type::NUM_TIME, "timeout", "the median time past of a block at which the deployment is considered failed if not yet locked in"},
1585+        {RPCResult::Type::NUM, "min_activation_height", "minimum height of blocks for which the rules may be enforced"},
1586+        {RPCResult::Type::STR, "status", "bip9 status of specified block (one of \"defined\", \"started\", \"locked_in\", \"active\", \"failed\")"},
1587+        {RPCResult::Type::NUM, "since", "height of the first block to which the status applies"},
1588+        {RPCResult::Type::STR, "status-next", "bip9 status of next block"},

jonatack commented at 5:40 pm on January 27, 2022:

Throughout the RPC documentation, when referring to the BIP itself and not a field name or input/output value

0        {RPCResult::Type::STR, "status-next", "BIP 9 status of the next block"},

ajtowns commented at 7:35 am on January 28, 2022:

Changed in #24187

jonatack commented at 5:49 pm on January 27, 2022: member

WIP, reviewed the first 3 commits and the release note. A few minor comments to pick/choose.

ajtowns commented at 7:36 am on January 28, 2022: member

Opened #24187 for non-code-changing followups.

MarcoFalke merged this on Jan 28, 2022

MarcoFalke closed this on Jan 28, 2022

MarcoFalke removed this from the "Blockers" column in a project

sidhujag referenced this in commit bedd40fa70 on Jan 28, 2022

MarcoFalke referenced this in commit af0b578041 on Feb 14, 2022

sidhujag referenced this in commit 2d957724f7 on Feb 14, 2022

MarcoFalke referenced this in commit 8270740bef on May 17, 2022

sidhujag referenced this in commit c64b45dc82 on May 28, 2022

Roasbeef referenced this in commit fabf989e5e on Aug 5, 2022

Roasbeef referenced this in commit 592af654d0 on Aug 5, 2022

Roasbeef referenced this in commit 3a37ef3cb8 on Aug 5, 2022

Roasbeef referenced this in commit 0718e5d6ee on Aug 6, 2022

Roasbeef referenced this in commit 492f8b6999 on Aug 6, 2022

Roasbeef referenced this in commit deeaa0c9c7 on Aug 13, 2022

achow101 referenced this in commit 92be831847 on Oct 13, 2022

sidhujag referenced this in commit 151c348370 on Oct 23, 2022

DrahtBot locked this on Jan 28, 2023

Add getdeploymentinfo RPC #23508

Conflicts