RPC: getblockchaininfo returns BIP signaling statistics

[pinheadmz]

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]

[gmaxwell]

NAK. The BIP9 quorum sensing is not a vote.

[pinheadmz]

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]

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

[pinheadmz]

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

[gmaxwell]

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

[pinheadmz]

@gmaxwell Added tests, look ok?

[TheBlueMatt]

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

[sipa]

Agree.

[TheBlueMatt]

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

[instagibbs]

concept ACK

[instagibbs]

remove?

[instagibbs]

try to avoid changing whitespace randomly

[TheBlueMatt]

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

[robmcl4]

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

[pinheadmz]

@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

[sipa]

Agree.

[sipa]

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

[pinheadmz]

already gone :)

[sipa]

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]

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

[pinheadmz]

squashed

[jnewbery]

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.

[jnewbery]

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

[jnewbery]

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

[jnewbery]

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'.

[jnewbery]

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]

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]

yep, sounds fine to me.

[pinheadmz]

Actually just took out declarations for all those variables, assigned to return object directly and refer to those assignments downstream. (see 6eecfe0)

[jnewbery]

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]

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]

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

[jnewbery]

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]

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

[jnewbery]

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

[pinheadmz]

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]

ok

[jnewbery]

combine these lines?

[jnewbery]

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]

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]

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.

[luke-jr]

Do we really want spaces in keys?

[pinheadmz]

Whoops, thanks. Fixed and squashed

[sipa]

utACK 83538dd

[TheBlueMatt]

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]

thanks

[TheBlueMatt]

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]

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 ?

[TheBlueMatt]

I believe this should be >=?

[pinheadmz]

good catch thanks

[pinheadmz]

I'll update my test to hit the edge too

[laanwj]

s/staistics/statistics/

[pinheadmz]

thanks, fixed

[TheBlueMatt]

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]

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]

@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

[TheBlueMatt]

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

[kallewoof]

utACK 9b9b685

[kallewoof]

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.

[pinheadmz]

rebased on master a0b1e57

[gmaxwell]

Concept ACK