RPC: augment getblockchaininfo bip9

mruddy commented at 2:41 PM on April 26, 2016: contributor

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.

in src/main.cpp:None in db83550940 outdated

5951 | @@ -5952,6 +5952,20 @@ ThresholdState VersionBitsTipState(const Consensus::Params& params, Consensus::D
5952 |      return VersionBitsState(chainActive.Tip(), params, pos, versionbitscache);
5953 |  }
5954 |  
5955 | +const CBlockIndex* VersionBitsLockedInBlock(const Consensus::DeploymentPos pos)
5956 | +{
5957 | +    LOCK(cs_main);
5958 | +    ThresholdConditionCache cache = versionbitscache.caches[pos];

sipa commented at 2:43 PM on April 26, 2016:

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

mruddy commented at 3:32 PM on April 26, 2016:

done, thanks

mruddy force-pushed on Apr 26, 2016

paveljanik commented at 8:31 PM on April 26, 2016: contributor

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

mruddy commented at 9:21 PM on April 26, 2016: contributor

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 commented at 9:24 PM on April 26, 2016: 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 commented at 10:39 PM on April 26, 2016: contributor

@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 force-pushed on Apr 27, 2016

mruddy renamed this:
~~RPC: add locked_in block hash to getblockchaininfo bip9_softforks data~~
RPC: augment getblockchaininfo bip9_softforks data
on Apr 27, 2016

mruddy commented at 2:00 PM on April 27, 2016: contributor

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 commented at 3:51 PM on April 27, 2016: contributor

Travis problem unrelated (wallet.py).

ACK https://github.com/bitcoin/bitcoin/pull/7948/commits/4ba7830bbedbd1739056f80a347f1d4553328091

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/).

MarcoFalke added the label RPC/REST/ZMQ on Apr 27, 2016

sdaftuar commented at 6:01 PM on April 28, 2016: member

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 force-pushed on Apr 29, 2016

mruddy commented at 12:57 AM on April 29, 2016: contributor

@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 force-pushed on Apr 29, 2016

mruddy commented at 10:50 AM on May 5, 2016: contributor

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

in src/main.cpp:None in ef4332ef49 outdated

5951 | @@ -5952,6 +5952,30 @@ ThresholdState VersionBitsTipState(const Consensus::Params& params, Consensus::D
5952 |      return VersionBitsState(chainActive.Tip(), params, pos, versionbitscache);
5953 |  }
5954 |  
5955 | +int VersionBitsStateBeginningBlockHeight(const Consensus::DeploymentPos pos, const ThresholdState thresholdState)

sipa commented at 4:15 PM on May 5, 2016:

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

sipa commented at 4:18 PM on May 5, 2016:

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 force-pushed on May 6, 2016

mruddy commented at 6:31 PM on May 6, 2016: contributor

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

mruddy force-pushed on May 6, 2016

in src/versionbits.cpp:None in 62934242e8 outdated

 105 | +    // if we are computing for the last block of a period, then pindexPrev points to the second to last block of the period, and
 106 | +    // if we are computing for the first block of a period, then pindexPrev points to the last block of the previous period.
 107 | +    // The parent of the genesis block is represented by NULL.
 108 | +    pindexPrev = pindexPrev->GetAncestor(pindexPrev->nHeight - ((pindexPrev->nHeight + 1) % nPeriod));
 109 | +
 110 | +    const CBlockIndex* previousPeriodParent = pindexPrev->GetAncestor(pindexPrev->nHeight - nPeriod);

sipa commented at 7:35 PM on May 9, 2016:

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

mruddy commented at 10:05 PM on May 9, 2016:

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 commented at 10:45 AM on May 13, 2016:

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

sipa commented at 9:02 PM on May 16, 2016:

Yes, agree.

mruddy commented at 10:27 AM on May 16, 2016: contributor

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

sipa commented at 9:02 PM on May 16, 2016: member

utACK 62934242e85d693de13d31d84da80e6f9bc2927b

luke-jr commented at 3:36 PM on June 2, 2016: member

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 commented at 3:45 PM on June 2, 2016: member

@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 commented at 3:59 PM on June 2, 2016: contributor

ACK https://github.com/bitcoin/bitcoin/pull/7948/commits/62934242e85d693de13d31d84da80e6f9bc2927b

laanwj commented at 10:12 AM on June 16, 2016: member

Needs rebase.

laanwj added the label Feature on Jun 16, 2016

mruddy force-pushed on Jun 16, 2016

mruddy commented at 12:25 PM on June 16, 2016: contributor

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: https://github.com/mruddy/bitcoin/commit/62934242e85d693de13d31d84da80e6f9bc2927b

mruddy commented at 12:05 PM on August 24, 2016: contributor

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

mruddy commented at 1:37 PM on August 24, 2016: contributor

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

mruddy force-pushed on Sep 4, 2016

mruddy commented at 12:52 PM on September 4, 2016: contributor

rebased to stay current and get travis green again

mruddy force-pushed on Oct 5, 2016

mruddy force-pushed on Oct 17, 2016

mruddy commented at 9:24 PM on October 18, 2016: contributor

@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 commented at 7:34 AM on October 19, 2016: member

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)

RPC: augment getblockchaininfo bip9_softforks data fc146095d2

mruddy force-pushed on Oct 19, 2016

mruddy commented at 2:17 PM on October 19, 2016: contributor

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

laanwj merged this on Oct 19, 2016

laanwj closed this on Oct 19, 2016

laanwj referenced this in commit 5d2c8e524e on Oct 19, 2016

mruddy deleted the branch on Oct 19, 2016

luke-jr referenced this in commit 46b1813c89 on Oct 20, 2016

mruddy commented at 11:52 AM on January 18, 2017: contributor

@pinheadmz I received your question: "Are you sure this offset is necessary? In regtest mode on my machine, segwit status is "started" on block 143, but this RPC call returns "since": 144 even though that block height has not been generated yet."

Answer: When you start a fresh regtest node (regtest uses a retarget interval of 144 blocks), you have 1 block @ height 0 (the genesis block) and the bip9 softfork "segwit" is "defined". Generate 142 blocks, segwit still defined. Generate 1 segwit is now started. So, we've generated 143 blocks and have 144 total. That is one retarget interval of blocks. When we have 144 blocks in the chain (a whole interval) BIP 9 can know what status the next block will be. getblockchaininfo.since is defined as the "height of the first block to which the status applies". Height 144 is the 145th block in the chain. From BIP 9: "Given that the state for a specific block/deployment combination is completely determined by its ancestry before the current retarget period (i.e. up to and including its ancestor with height block.height - 1 - (block.height % 2016))". So, at height 143, we know what the status will be for height 144 and it'll be the new status of "started" which is the "height of the first block to which the status applies".

pinheadmz commented at 4:49 PM on January 18, 2017: member

@mruddy Thanks! I also just saw the offset was already discussed earlier in the thread, it makes sense.

codablock referenced this in commit 99cc6b3bcb on Sep 19, 2017

codablock referenced this in commit dc9556a890 on Jan 13, 2018

andvgal referenced this in commit c0695897d0 on Jan 6, 2019

CryptoCentric referenced this in commit 3e3078af83 on Feb 15, 2019

MarcoFalke locked this on Sep 8, 2021

RPC: augment getblockchaininfo bip9_softforks data #7948