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

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

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

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

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.

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

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.

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

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

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

utACK 62934242e85d693de13d31d84da80e6f9bc2927b

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

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

ACK 6293424

Needs rebase.

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

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

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

rebased to stay current and get travis green again

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

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)

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

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

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