RPC: getblockchaininfo returns BIP signaling statistics #9571

pinheadmz commented at 1:27 AM on January 18, 2017: member

Adds a new array to getblockchaininfo that returns the number of blocks in the current period signaling a BIP9 condition.

Example:

"segwit": {
      "status": "started",
      "bit": 1,
      "startTime": 0,
      "timeout": 999999999999,
      "since": 144,
      "statistics": {
        "period length": 144,
        "threshold": 108,
        "elapsed": 143,
        "count": 143,
        "possible": true
      }
}

and the explanation:

"statistics": {        (array) numeric statistics about BIP9 signalling for a softfork
   "period length": xx, (numeric) the length in blocks of the BIP9 signalling period 
   "threshold": xx,    (numeric) the number of blocks with the version bit set required to activate the feature 
   "elapsed": xx,      (numeric) the number of blocks elapsed since the beginning of the current period 
   "count": xx         (numeric) the number of blocks with the version bit set in the current period 
   "possible": xx      (boolean) returns false if there are not enough blocks left in this period to pass activation threshold 
}

[related question]

in src/rpc/blockchain.cpp:None in f11577c160 outdated

 186 | @@ -187,7 +187,7 @@ void RPCNotifyBlockChange(bool ibd, const CBlockIndex * pindex)
 187 |          latestblock.hash = pindex->GetBlockHash();
 188 |          latestblock.height = pindex->nHeight;
 189 |      }
 190 | -	cond_blockchange.notify_all();
 191 | +    cond_blockchange.notify_all();

instagibbs commented at 3:15 AM on January 18, 2017:

try to avoid changing whitespace randomly

TheBlueMatt commented at 10:48 PM on January 18, 2017:

That wasnt random, that was replacing an errant tab, I say leave it.

in .gitignore:None in f11577c160 outdated

   7 | @@ -8,6 +8,7 @@ src/bitcoin-tx
   8 |  src/test/test_bitcoin
   9 |  src/test/test_bitcoin_fuzzy
  10 |  src/qt/test/test_bitcoin-qt
  11 | +src/datadir

instagibbs commented at 3:17 AM on January 18, 2017:

remove?

pinheadmz force-pushed on Jan 18, 2017

fanquake added the label RPC/REST/ZMQ on Jan 18, 2017

gmaxwell commented at 9:03 AM on January 18, 2017: contributor

NAK. The BIP9 quorum sensing is not a vote.

pinheadmz commented at 5:03 PM on January 18, 2017: member

Signaling statistics for soft forks pre-BIP9 used to be reported by getblockchaininfo.

I think it's helpful to see the adoption rate to better predict when a soft fork will engage. Currently there are several 3rd party websites that report these statistics, I would rather get them from my own node.

sipa commented at 5:06 PM on January 18, 2017: member

I think these are useful statistics to have. Just don't call it a vote in the RPC help.

pinheadmz commented at 5:14 PM on January 18, 2017: member

@sipa no V-word in the RPC help. I rephrased my commit description.

gmaxwell commented at 8:14 PM on January 18, 2017: contributor

yea, okay. Anyone interested in writing a test for one off on the possible boundary.

pinheadmz force-pushed on Jan 18, 2017

pinheadmz commented at 10:00 PM on January 18, 2017: member

@gmaxwell Added tests, look ok?

in src/validation.h:None in 69cff857e5 outdated

  31 | @@ -32,6 +32,8 @@
  32 |  #include <boost/unordered_map.hpp>
  33 |  #include <boost/filesystem/path.hpp>
  34 |  
  35 | +#include <univalue.h>

TheBlueMatt commented at 10:01 PM on January 18, 2017:

Preferably dont add a UniValue dependancy here...maybe just define a struct which carries the info you want?

sipa commented at 1:04 AM on January 19, 2017:

Agree.

TheBlueMatt commented at 10:01 PM on January 18, 2017: member

Looks cool, I've previously written python crap to calculate similar stats from rpc data.

instagibbs commented at 10:17 PM on January 18, 2017: member

concept ACK

in src/versionbits.cpp:None in d4a506dea8 outdated

 127 | +        obj.push_back(Pair("elapsed", elapsedBlocks));
 128 | +    }
 129 | +    
 130 | +    // Walk back from current block to beginning of period
 131 | +    std::vector<const CBlockIndex*> vToCompute;
 132 | +    while (pindexPrev->nHeight != currentIndex->nHeight){

robmcl4 commented at 10:29 PM on January 18, 2017:

What if pindexPrev == currentIndex == NULL? Also, why not keep a rolling count rather than using an accessory vector?

pinheadmz commented at 11:26 PM on January 18, 2017: member

@TheBlueMatt UniValue replaced with struct (although not sure I returned the values the most elegant way) @robmcl4 Thanks, took out vector and just counted. If pindexPrev == currentIndex == NULL we're on the genesis block and count just stays at 0 anyway. @instagibbs Removed my extra ignore line but left tab fix

pinheadmz force-pushed on Jan 18, 2017

in src/versionbits.cpp:None in 69cff857e5 outdated

   2 | @@ -3,8 +3,9 @@
   3 |  // file COPYING or http://www.opensource.org/licenses/mit-license.php.
   4 |  
   5 |  #include "versionbits.h"
   6 | -
   7 |  #include "consensus/params.h"
   8 | +#include <boost/foreach.hpp>

sipa commented at 1:10 AM on January 19, 2017:

Please don't add new boost foreach uses, use c++11 range-based for instead.

pinheadmz commented at 1:32 AM on January 19, 2017:

already gone :)

in src/versionbits.cpp:None in 69cff857e5 outdated

 105 | @@ -105,6 +106,47 @@ ThresholdState AbstractThresholdConditionChecker::GetStateFor(const CBlockIndex*
 106 |      return state;
 107 |  }
 108 |  
 109 | +// return the numerical statistics of blocks signalling the specified BIP9 condition in this current period
 110 | +UniValue AbstractThresholdConditionChecker::GetStateStatisticsFor(const CBlockIndex* pindexPrev, const Consensus::Params& params, ThresholdConditionCache& cache) const

sipa commented at 1:17 AM on January 19, 2017:

Does this code need to be in the condition checker implementation? It seems like it could be written generically, as it doesn't need access to the cache or internal state machine.

pinheadmz commented at 1:34 AM on January 19, 2017:

Should I take it out? VersionBitsConditionChecker already has Condition() function for the bitmask for the corresponding soft fork.

pinheadmz force-pushed on Jan 20, 2017

pinheadmz commented at 7:19 PM on January 20, 2017: member

squashed

in src/rpc/blockchain.cpp:None in 2d93e3c995 outdated

1118 | +            "        \"since\": xx,           (numeric) height of the first block to which the status applies\n"
1119 | +            "        \"statistics\": {        (object) numeric statistics about BIP9 signalling for a softfork\n"
1120 | +            "           \"period length\": xx, (numeric) the length in blocks of the BIP9 signalling period \n"
1121 | +            "           \"threshold\": xx,    (numeric) the number of blocks with the version bit set required to activate the feature \n"
1122 | +            "           \"elapsed\": xx,      (numeric) the number of blocks elapsed since the beginning of the current period \n"
1123 | +            "           \"count\": xx         (numeric) the number of blocks with the version bit set in the current period \n"

jnewbery commented at 8:48 PM on January 20, 2017:

nanonit: comma after \"count\": xx please

in src/versionbits.cpp:None in 2d93e3c995 outdated

 103 | @@ -105,6 +104,42 @@ ThresholdState AbstractThresholdConditionChecker::GetStateFor(const CBlockIndex*
 104 |      return state;
 105 |  }
 106 |  
 107 | +// return the numerical statistics of blocks signalling the specified BIP9 condition in this current period
 108 | +BIP9Stats AbstractThresholdConditionChecker::GetStateStatisticsFor(const CBlockIndex* pindexPrev, const Consensus::Params& params) const
 109 | +{
 110 | +    BIP9Stats obj;

jnewbery commented at 9:18 PM on January 20, 2017:

nit: I suspect the name 'obj' is from when you were passing the object back as a univalue. I'd prefer for this variable to be named a little more descriptively, perhaps 'stats' or 'bip9Stats'.

in src/rpc/blockchain.cpp:None in 2d93e3c995 outdated

1113 | @@ -1102,7 +1114,14 @@ UniValue getblockchaininfo(const JSONRPCRequest& request)
1114 |              "        \"bit\": xx,             (numeric) the bit (0-28) in the block version field used to signal this softfork (only for \"started\" status)\n"
1115 |              "        \"startTime\": xx,       (numeric) the minimum median time past of a block at which the bit gains its meaning\n"
1116 |              "        \"timeout\": xx,         (numeric) the median time past of a block at which the deployment is considered failed if not yet locked in\n"
1117 | -            "        \"since\": xx            (numeric) height of the first block to which the status applies\n"
1118 | +            "        \"since\": xx,           (numeric) height of the first block to which the status applies\n"
1119 | +            "        \"statistics\": {        (object) numeric statistics about BIP9 signalling for a softfork\n"

jnewbery commented at 9:26 PM on January 20, 2017:

nit: perhaps add (only for \"started\" status)\n (like for the bit field above)

in src/versionbits.cpp:None in 2d93e3c995 outdated

 133 | +        currentIndex = currentIndex->pprev;
 134 | +    }
 135 | +
 136 | +    obj.count = count;
 137 | +    bool possible = (nPeriod - nThreshold) > (elapsedBlocks - count);
 138 | +    obj.possible = possible;

jnewbery commented at 9:36 PM on January 20, 2017:

combine these lines?

in src/versionbits.cpp:None in 2d93e3c995 outdated

 117 | +    
 118 | +    obj.period = nPeriod;
 119 | +    obj.threshold = nThreshold;
 120 | +
 121 | +    // Find beginning of period
 122 | +    if (pindexPrev != NULL) {

jnewbery commented at 9:39 PM on January 20, 2017:

You're checking that pindexPrev isn't NULL, but then dereferencing further down (in line 130) even if this check fails.

Perhaps move this check to the top and return from the function if pindexPrev is NULL?

pinheadmz commented at 6:03 PM on January 23, 2017:

Moved the check up so the return struct is partially initialized. However, this function shouldn't ever be called unless THRESHOLD_STARTED == thresholdState anyway, which will never be the case at block 0.

jnewbery commented at 6:36 PM on January 23, 2017:

I agree that this should always be non-NULL, but it's better to be conservative, and it's certainly better to be consistently conservative than conservative here and not conservative later :)

in src/versionbits.cpp:None in 2d93e3c995 outdated

 120 | +
 121 | +    // Find beginning of period
 122 | +    if (pindexPrev != NULL) {
 123 | +        pindexPrev = pindexPrev->GetAncestor(pindexPrev->nHeight - ((pindexPrev->nHeight + 1) % nPeriod));
 124 | +        elapsedBlocks = currentIndex->nHeight - pindexPrev->nHeight;
 125 | +        obj.elapsed = elapsedBlocks;

jnewbery commented at 9:41 PM on January 20, 2017:

Is it tidier to calculate elapsed directly from (pindexPrev % nPeriod) + 1?

pinheadmz commented at 6:03 PM on January 23, 2017:

Not sure about this. After making this one line, it seems the same to me. Still need to get nHeight from each. Simpler to me to see "Current - Start = Elapsed"

jnewbery commented at 6:39 PM on January 23, 2017:

ok

in src/versionbits.cpp:None in 2d93e3c995 outdated

 109 | +{
 110 | +    BIP9Stats obj;
 111 | +    
 112 | +    int elapsedBlocks;
 113 | +    int nPeriod = Period(params);
 114 | +    int nThreshold = Threshold(params);

jnewbery commented at 9:47 PM on January 20, 2017:

You're declaring local variables, setting them once, and then setting the obj members to those values. Would it be cleaner to just set the obj members directly?

pinheadmz commented at 6:03 PM on January 23, 2017:

nPeriod and nThreshold are used again in line 137 (now line 135) to calculate the possible value. But you are right about elapsedBlocks and the actual bool possible, so I took those declarations out and simplified the assignments.

jnewbery commented at 6:33 PM on January 23, 2017:

yep, sounds fine to me.

pinheadmz commented at 7:52 PM on January 23, 2017:

Actually just took out declarations for all those variables, assigned to return object directly and refer to those assignments downstream. (see https://github.com/bitcoin/bitcoin/pull/9571/commits/6eecfe0368525f70ff8b93914320356ae0070a0c)

in src/versionbits.h:None in 2d93e3c995 outdated

  60 | @@ -53,6 +61,7 @@ class AbstractThresholdConditionChecker {
  61 |  public:
  62 |      // Note that the functions below take a pindexPrev as input: they compute information for block B based on its parent.

jnewbery commented at 11:03 PM on January 20, 2017:

This comment is very misleading, and so is naming the variable pindexPrev.

When GetStateFor() was written, the meaning of the function was: "I have block B in hand, and I'm trying to connect it to block A in the tree. What are the BIP9 softfork consensus rules that I should enforce on block B based on block A?" In that context, pindexPrev makes sense - we're judging block B's consensus rules based on its pindexPrev pointer to block A.

GetStateStatisticsFor() and GetStateSinceHeightFor() (introduced in #7948) are being used for a different purpose. The meaning of these functions is "Block A is our view of the current tip. What is our view of current network consensus rules?" In that context, pindexPrev doesn't make sense at all. There's no block B with a pindexPointer back to block A.

I'd prefer for the variable pindexPrev to be renamed in GetStateStatisticsFor() and GetStateSinceHeightFor(). Perhaps change it to pindex and update the comment to describe what the functions are actually being used for.

pinheadmz commented at 6:04 PM on January 23, 2017:

Yeah ok this makes sense to me. I'll take my function out of the "functions below" list and rename variable to pindex. Should correcting GetStateSinceHeightFor() happen in a different commit though?

jnewbery commented at 6:43 PM on January 23, 2017:

yeah, I'd suggest changing GetStateSinceHeightFor() in a different commit. I can open a PR for that or leave it to you if you prefer.

in src/versionbits.cpp:None in 2d93e3c995 outdated

 118 | +    obj.period = nPeriod;
 119 | +    obj.threshold = nThreshold;
 120 | +
 121 | +    // Find beginning of period
 122 | +    if (pindexPrev != NULL) {
 123 | +        pindexPrev = pindexPrev->GetAncestor(pindexPrev->nHeight - ((pindexPrev->nHeight + 1) % nPeriod));

jnewbery commented at 2:28 PM on January 23, 2017:

I don't much like this reassignment of pindexPrev to the first block in the current difficulty window. I know that the same thing happens in AbstractThresholdConditionChecker() above, but I think it'd be clearer to have a local variable called something like pindexFirstInWindow. Reasoning about whether a function is doing what you want is easier if the variables are named what they're actually used for.

pinheadmz commented at 6:03 PM on January 23, 2017:

Yes I was mostly copying the format of AbstractThresholdConditionChecker::GetStateFor() but I agree with you, changed variable name.

jnewbery commented at 3:32 PM on January 23, 2017: member

Concept ACK, and functional ACK. I've tested this and I'm fairly confident it's calculating the values correctly. Thanks for adding the additional tests to bip9-softforks.py

I have a few code style and variable naming nits.

pinheadmz force-pushed on Jan 23, 2017

in src/rpc/blockchain.cpp:None in 73971ada1b outdated

1056 | @@ -1057,6 +1057,18 @@ static UniValue BIP9SoftForkDesc(const Consensus::Params& consensusParams, Conse
1057 |      rv.push_back(Pair("startTime", consensusParams.vDeployments[id].nStartTime));
1058 |      rv.push_back(Pair("timeout", consensusParams.vDeployments[id].nTimeout));
1059 |      rv.push_back(Pair("since", VersionBitsTipStateSinceHeight(consensusParams, id)));
1060 | +    if (THRESHOLD_STARTED == thresholdState)
1061 | +    {
1062 | +        UniValue statsUV(UniValue::VOBJ);
1063 | +        BIP9Stats statsStruct = VersionBitsTipStatistics(consensusParams, id);
1064 | +        statsUV.push_back(Pair("period length", statsStruct.period));

luke-jr commented at 9:22 PM on February 2, 2017:

Do we really want spaces in keys?

pinheadmz commented at 10:08 PM on February 2, 2017:

Whoops, thanks. Fixed and squashed

pinheadmz force-pushed on Feb 2, 2017

sipa commented at 7:01 AM on February 10, 2017: member

utACK 83538dd86dc426dc252714d4ca1ae91ad2c5ac24

laanwj added this to the milestone 0.15.0 on Feb 10, 2017

in src/rpc/blockchain.cpp:None in 83538dd86d outdated

1064 | +        statsUV.push_back(Pair("period", statsStruct.period));
1065 | +        statsUV.push_back(Pair("threshold", statsStruct.threshold));
1066 | +        statsUV.push_back(Pair("elapsed", statsStruct.elapsed));
1067 | +        statsUV.push_back(Pair("count", statsStruct.count));
1068 | +        statsUV.push_back(Pair("possible", statsStruct.possible));
1069 | +

TheBlueMatt commented at 6:16 PM on February 10, 2017:

Please be careful to not introduce lines with whitespace at the end. Looks like this patch has it in 5 places (here and 4 lines in AbstractThreasholdConditionCHecker::GetStateStatisticsFor).

pinheadmz commented at 7:14 PM on February 10, 2017:

thanks

in src/versionbits.cpp:None in 83538dd86d outdated

 114 | +    
 115 | +    if (pindex == NULL)
 116 | +        return stats;
 117 | +
 118 | +    // Find beginning of period
 119 | +    const CBlockIndex* pindexFirstInPeriod = pindex->GetAncestor(pindex->nHeight - ((pindex->nHeight + 1) % stats.period));

TheBlueMatt commented at 6:51 PM on February 10, 2017:

Can we get a more descriptive name? This is actually the last block in the previous period, not the first in this period (though it looks like all the later math is correct).

pinheadmz commented at 7:18 PM on February 10, 2017:

Ah I think you're right. I copied this line from AbstractThresholdConditionChecker::GetStateFor

https://github.com/pinheadmz/bitcoin/blob/83538dd86dc426dc252714d4ca1ae91ad2c5ac24/src/versionbits.cpp#L30-L33

We'll call it pindexEndOfPrevPeriod ?

in src/versionbits.cpp:None in 83538dd86d outdated

 127 | +            count++;
 128 | +        currentIndex = currentIndex->pprev;
 129 | +    }
 130 | +
 131 | +    stats.count = count;
 132 | +    stats.possible = (stats.period - stats.threshold ) > (stats.elapsed - count);

TheBlueMatt commented at 6:53 PM on February 10, 2017:

I believe this should be >=?

pinheadmz commented at 7:14 PM on February 10, 2017:

good catch thanks

pinheadmz commented at 7:21 PM on February 10, 2017:

I'll update my test to hit the edge too

pinheadmz force-pushed on Feb 10, 2017

in src/validation.h:None in 330e8a1f6e outdated

 329 | @@ -330,6 +330,9 @@ std::string FormatStateMessage(const CValidationState &state);
 330 |  /** Get the BIP9 state for a given deployment at the current tip. */
 331 |  ThresholdState VersionBitsTipState(const Consensus::Params& params, Consensus::DeploymentPos pos);
 332 |  
 333 | +/** Get the numerical staistics for the BIP9 state for a given deployment at the current tip. */

laanwj commented at 4:28 PM on February 20, 2017:

s/staistics/statistics/

pinheadmz commented at 10:18 PM on February 21, 2017:

thanks, fixed

in qa/rpc-tests/bip9-softforks.py:None in 330e8a1f6e outdated

 141 |          # using a variety of bits to simulate multiple parallel softforks
 142 | -        test_blocks = self.generate_blocks(50, activated_version) # 0x20000001 (signalling ready)
 143 | -        test_blocks = self.generate_blocks(20, 4, test_blocks) # 0x00000004 (signalling not)
 144 | -        test_blocks = self.generate_blocks(50, activated_version, test_blocks) # 0x20000101 (signalling ready)
 145 | -        test_blocks = self.generate_blocks(24, 4, test_blocks) # 0x20010000 (signalling not)
 146 | +        test_blocks = self.generate_blocks(40, activated_version) # 0x20000001 (signalling ready)

TheBlueMatt commented at 7:29 PM on February 20, 2017:

I'd kinda prefer if you didnt change Test 2 here...just add more tests as "Test 1" with a 144-block-total. Also, though I /think/ the new code is right, it'd be really nice if you could add two tests: one which fails to active by 1 block (and checks the "possible" flag is false after the first N), and one which activates with one more signaling block, to make sure the possible flat is set right.

pinheadmz commented at 10:05 PM on February 20, 2017:

I think I see what you mean, test the flag at the end of the period to make sure the flag matches the actual outcome in the end? That's cool for the negative test, but since it can only activate once, the positive test must borrow some blocks from Test 2?

pinheadmz commented at 10:20 PM on February 21, 2017:

@TheBlueMatt I rearranged the tests to hit the thresholds. I put Test 2 back to where it was, but still had to modify Test 3 to cover everything (just borrowed one block and tested the possible flag right before period completion). LMK what you think, thanks

pinheadmz force-pushed on Feb 21, 2017

TheBlueMatt commented at 4:27 PM on February 22, 2017: member

utACK 9b9b68527dc95a1edca7de24286473ac9a59499c thanks so much for sticking with this :).

in src/validation.h:None in 9b9b68527d outdated

 329 | @@ -330,6 +330,9 @@ std::string FormatStateMessage(const CValidationState &state);
 330 |  /** Get the BIP9 state for a given deployment at the current tip. */
 331 |  ThresholdState VersionBitsTipState(const Consensus::Params& params, Consensus::DeploymentPos pos);
 332 |  
 333 | +/** Get the numerical statistics for the BIP9 state for a given deployment at the current tip. */
 334 | +BIP9Stats VersionBitsTipStatistics(const Consensus::Params& params, Consensus::DeploymentPos pos);

kallewoof commented at 12:35 AM on March 1, 2017:

Nit: I keep getting this and VersionBitsStatistics mixed up when I read through the changes. I think it would help readability to name this e.g. CurrentVersionBitsStatistics or ActiveVersionBitsStatistics or something.

kallewoof commented at 12:36 AM on March 1, 2017: member

utACK 9b9b685

RPC: getblockchaininfo: BIP9 stats

add RPC tests for BIP9 counting stats

557c9a68fb

pinheadmz force-pushed on Mar 24, 2017

pinheadmz commented at 12:00 AM on March 25, 2017: member

rebased on master a0b1e57b20a17177ed5a9a54e4a8aab597a546b4

gmaxwell commented at 12:15 AM on March 28, 2017: contributor

Concept ACK

laanwj merged this on May 23, 2017

laanwj closed this on May 23, 2017

laanwj referenced this in commit 46771514fa on May 23, 2017

PastaPastaPasta referenced this in commit 56b61df106 on Jun 10, 2019

PastaPastaPasta referenced this in commit 182263b623 on Jun 11, 2019

PastaPastaPasta referenced this in commit 637adf2f0e on Jun 11, 2019

PastaPastaPasta referenced this in commit 93adad0995 on Jun 15, 2019

PastaPastaPasta referenced this in commit 66e00895d7 on Jun 19, 2019

PastaPastaPasta referenced this in commit 866e63e4bc on Jun 19, 2019

PastaPastaPasta referenced this in commit e9d5851965 on Jun 19, 2019

PastaPastaPasta referenced this in commit ecdc942872 on Jun 19, 2019

PastaPastaPasta referenced this in commit 18fef82a32 on Jun 19, 2019

PastaPastaPasta referenced this in commit d94aac7bd3 on Jun 20, 2019

PastaPastaPasta referenced this in commit eb21d9ed55 on Jun 22, 2019

PastaPastaPasta referenced this in commit 2591043c5a on Jun 22, 2019

PastaPastaPasta referenced this in commit d7edeb0b12 on Jun 22, 2019

PastaPastaPasta referenced this in commit 013538dadb on Jun 22, 2019

PastaPastaPasta referenced this in commit 9fc019b06e on Jun 22, 2019

PastaPastaPasta referenced this in commit 324c588db8 on Jun 24, 2019

barrystyle referenced this in commit cbe725a85f on Jan 22, 2020

DrahtBot locked this on Sep 8, 2021