RPC: augment getblockchaininfo bip9_softforks data #7948

Merged
merged 1 commit into from Oct 19, 2016

Projects

None yet

7 participants

@mruddy
Contributor
mruddy commented Apr 26, 2016

This adds the hash of the first block of the LOCKED_IN period for a given deployment (when the deployment is LOCKED_IN or ACTIVE) to the bip9_softforks data of the getblockchaininfo RPC.

I implemented this as a simple scan through the already existing map values to make this change easy to review. I looked at adding an array to the version bits cache struct and then populating it from the THRESHOLD_LOCKED_IN case in AbstractThresholdConditionChecker::GetStateFor, but that would have meant more refactoring.

The motivation for this is that I was researching some forking going on on testnet3 and one of the first questions I had was, "When were the BIP9 forks locking in and activating?" This patch provides the info necessary to begin digging around and figuring that out. This provides the lock in block. Take its height and then mentally add 2016 to get the ACTIVE block height etc... This was the simplest change to get the info I was after.

@sipa sipa and 1 other commented on an outdated diff Apr 26, 2016
src/main.cpp
@@ -5952,6 +5952,20 @@ ThresholdState VersionBitsTipState(const Consensus::Params& params, Consensus::D
return VersionBitsState(chainActive.Tip(), params, pos, versionbitscache);
}
+const CBlockIndex* VersionBitsLockedInBlock(const Consensus::DeploymentPos pos)
+{
+ LOCK(cs_main);
+ ThresholdConditionCache cache = versionbitscache.caches[pos];
@sipa
sipa Apr 26, 2016 Member

Use a reference here; no need to copy the entire cache

@mruddy
mruddy Apr 26, 2016 Contributor

done, thanks

@paveljanik
Contributor

Why do you prefer to print block hash instead of block height?

@mruddy
Contributor
mruddy commented Apr 26, 2016

No real strong reason. Just that the height does not uniquely identify a block like its hash does, and if you want to lookup the block info (including its height), all you have to do after you have the hash is getblock <hash> instead of getblockhash <height> then getblock <hash>.

@paveljanik
Contributor

@mruddy Sure. But in this case, the lock in happens at height, not in the particular block hash - it can be reorganized. No?

@mruddy
Contributor
mruddy commented Apr 26, 2016

@paveljanik Yes, that's usually going to be the case. It would take an extraordinarily long re-org to change the height too (testnet style shenanigans). Height may be more intuitive to use here in that sense. If I made that change, I'd probably rename the attribute "lockedInHeight" too. I might do this tomorrow pending other feedback that that's not a good idea for some reason.

@mruddy mruddy changed the title from RPC: add locked_in block hash to getblockchaininfo bip9_softforks data to RPC: augment getblockchaininfo bip9_softforks data Apr 27, 2016
@mruddy
Contributor
mruddy commented Apr 27, 2016

Changed my mind on how this should work. Now a new member named "since" is part of the BIP9 data and it gives the height of the first block to which the current deployment status applied. Seems like this might be more useful.

@paveljanik
Contributor

Travis problem unrelated (wallet.py).

ACK 4ba7830

Thanks!

  "bip9_softforks": {
    "csv": {
      "status": "active",
      "startTime": 1456790400,
      "timeout": 1493596800,
      "since": 770111
    }
  }

This corresponds with the activation (https://www.reddit.com/r/Bitcoin/comments/4f4210/the_bip9_versionbits_csv_softfork_of_bip68_bip112/).

@sdaftuar
Contributor

The "since" height is off-by-one, as the rules wouldn't have gone into effect until block height 770112 (all the version bits state calculations take the prev block as an argument, so 770111 was the height of the last block before the rules were activated).

Also, when walking the versionbitscache, we should skip over entries that are not on chainActive, as presumably the question we're interested in answering is: at what height was the deployment activated on the current chain?

Concept ACK.

@mruddy
Contributor
mruddy commented Apr 29, 2016

@sdaftuar Wow, great code review, thanks! You're right. Geez, I totally missed that. Updates made. Added tests. I also used some C++11 in the latest updates because that was enabled earlier today.

@mruddy
Contributor
mruddy commented May 5, 2016

@sipa now that this is stable (hasn't changed in about a week) are you ok with adding this bip9 data in this way?

@sipa sipa commented on an outdated diff May 5, 2016
src/main.cpp
@@ -5952,6 +5952,30 @@ ThresholdState VersionBitsTipState(const Consensus::Params& params, Consensus::D
return VersionBitsState(chainActive.Tip(), params, pos, versionbitscache);
}
+int VersionBitsStateBeginningBlockHeight(const Consensus::DeploymentPos pos, const ThresholdState thresholdState)
@sipa
sipa May 5, 2016 Member

I think there is a more efficient implementation possible, but it would be cleaner to put it in AbstractThresholdConditionChecker, I think.

@sipa
sipa May 5, 2016 Member

AbstracrThresholdConditionChecker has the logic to walk the chain efficiently already (including first jumping back to a multiple of the period etc), so it could use the same and call GetStateFor for every multiple-or-period block back from the tip that was passed. That would also avoid breaking the abstraction that ThresholdConditionCache gives.

@mruddy
Contributor
mruddy commented May 6, 2016 edited

@sipa I restructured things how I believe you meant. I tried to follow existing patterns at the same time. Please check it when you get a chance. I also added more tests. Thanks!
EDIT: I just had an idea on how to do this with a little less code. I'll get that pushed up in a little bit if it works out.
EDIT2: ok, i guess that's good enough now.

@sipa sipa commented on the diff May 9, 2016
src/versionbits.cpp
+ // BIP 9 about state DEFINED: "The genesis block is by definition in this state for each deployment."
+ if (initialState == THRESHOLD_DEFINED) {
+ return 0;
+ }
+
+ const int nPeriod = Period(params);
+
+ // A block's state is always the same as that of the first of its period, so it is computed based on a pindexPrev whose height equals a multiple of nPeriod - 1.
+ // To ease understanding of the following height calculation, it helps to remember that
+ // right now pindexPrev points to the block prior to the block that we are computing for, thus:
+ // if we are computing for the last block of a period, then pindexPrev points to the second to last block of the period, and
+ // if we are computing for the first block of a period, then pindexPrev points to the last block of the previous period.
+ // The parent of the genesis block is represented by NULL.
+ pindexPrev = pindexPrev->GetAncestor(pindexPrev->nHeight - ((pindexPrev->nHeight + 1) % nPeriod));
+
+ const CBlockIndex* previousPeriodParent = pindexPrev->GetAncestor(pindexPrev->nHeight - nPeriod);
@sipa
sipa May 9, 2016 Member

Can't this cause a segfault if pindexPrev is NULL?

@mruddy
mruddy May 9, 2016 Contributor

There is protection against pindexPrev being NULL at that point though.

If this method gets called with the initial argument value of pindexPrev being NULL, or for a first period block (or more generally, for a block in any period that is in state DEFINED), then the first call of GetStateFor returns THRESHOLD_DEFINED and the method returns at line 97. Else, pindexPrev won't become NULL before previousPeriodParent does (which is what matters).

For example, consider a regtest network, where period=144, and our tip is at height 143 (i.e.- we're calling this before generating the block for height 144 and pindexPrev points to the tip at height=143), and we're now in STARTED (because the MTP time threshold was met), then line 108 leaves pindexPrev pointing at block with height=143 (idempotent when called for the first block in a period because pindexPrev already points at the correct block). Line 110 makes previousPeriodParent == NULL, but that is checked for by the loop (and only as a slight optimization as NULL could be passed into GetStateFor safely with the same effect).

@mruddy
mruddy May 13, 2016 Contributor

@sipa did that make sense? are we good now?

@sipa
sipa May 16, 2016 Member

Yes, agree.

@mruddy
Contributor
mruddy commented May 16, 2016

@laanwj I think this is ready. Do you agree?

@sipa
Member
sipa commented May 16, 2016

utACK 6293424

@luke-jr
Member
luke-jr commented Jun 2, 2016

IMO it would be nice to know at which block hash/height each transition was made. I think using the hash makes more sense, but not sure it matters much with BIP 9 (perhaps it might with some future softfork scheme?).

@sipa
Member
sipa commented Jun 2, 2016

@luke-jr By definition, consensus rules within a blockchain can only depend on its history, and not on that of other branches, so height is the only relevant value as it uniquely determines the block within the current chain. Furthermore, in BIP9, the status changes are never affected by the block itself (and often not by any of those in recent history before it either).

@paveljanik
Contributor

ACK 6293424

@laanwj
Member
laanwj commented Jun 16, 2016

Needs rebase.

@laanwj laanwj added the Feature label Jun 16, 2016
@mruddy
Contributor
mruddy commented Jun 16, 2016

rebase complete.
i only had to resolve minor changes to bip9-softforks.py. nothing else changed.
here's a link to the previous commit that got acks for a quick double check: mruddy@6293424

@mruddy
Contributor
mruddy commented Aug 24, 2016

@laanwj want to merge this? haven't changed since mid june. thanks!

@mruddy
Contributor
mruddy commented Aug 24, 2016

yep, that one-off travis failure looks unrelated to these changes. looks like something with compiling boost.

@mruddy
Contributor
mruddy commented Sep 4, 2016

rebased to stay current and get travis green again

@mruddy
Contributor
mruddy commented Oct 18, 2016

@laanwj if i rebase this to the 0.13 branch, would you merge it? if not, then should i close this pull? it's been unchanged for about four months and just got a conflict. the changes provide a little softfork info, but they also add a fair bit of code. i'm ok with either way you choose.

@laanwj
Member
laanwj commented Oct 19, 2016

This missed 0.13, and adds a feature, should be rebased to master instead.
(and truly sorry for it taking so long, but there's just so many pulls being opened all the time, I can't handle it anymore)

@mruddy
Contributor
mruddy commented Oct 19, 2016

rebased on master. no worries, thanks for all the good work you do.

@laanwj laanwj merged commit fc14609 into bitcoin:master Oct 19, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@laanwj laanwj added a commit that referenced this pull request Oct 19, 2016
@laanwj laanwj Merge #7948: RPC: augment getblockchaininfo bip9_softforks data
fc14609 RPC: augment getblockchaininfo bip9_softforks data (mruddy)
5d2c8e5
@mruddy mruddy deleted the mruddy:version_bits_locked_in_block branch Oct 19, 2016
@luke-jr luke-jr added a commit to bitcoinknots/bitcoin that referenced this pull request Oct 20, 2016
@mruddy @luke-jr mruddy + luke-jr RPC: augment getblockchaininfo bip9_softforks data
Github-Pull: #7948
Rebased-From: 1baa42a
46b1813
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment