RPC: add versionHex in getblock and getblockheader JSON results #7774

mruddy commented at 11:06 PM on March 30, 2016: contributor

Update getblock and getblockheader RPCs and rest to return hex encoded block versions, e.g.:

"version": "0x01"

"version": "0x20000000"

Reason is similar to #7763: "The decimal printing of block nVersion is not very readable in a versionbits world."

My only apprehension is that this could be disruptive to people that have script relying on this output. But, I'm not aware of any guarantee that this will not change.

I purposefully chose 02 instead of 08 in order to get a smaller encoding when possible.

sipa commented at 11:49 PM on March 30, 2016: member

I'd like to see this, but I'm not sure it's worth breaking the RPC interface for (something we generally try hard to avoid).

dcousens commented at 12:56 AM on March 31, 2016: contributor

Is the PRC designed for human (readable) consumption anyway? Converting these numbers to hex is often a 1-liner. This would also break a lot of code.

mruddy force-pushed on Mar 31, 2016

mruddy commented at 1:23 AM on March 31, 2016: contributor

Yes, these two RPCs have a verbose version (the default) that returns JSON. It's pretty useful to look something up quick via the debug window CLI in bitcoin-qt. The version makes more sense to me quickly when it's in hex, that's all. For example:

And yes, the type changes from number to string because in JSON, "the octal and hexadecimal formats are not used" [http://www.json.org/].

Actually, I suppose that just adding this as "version_hex" would be just as well. I might do that later.

dcousens commented at 1:41 AM on March 31, 2016: contributor

@mruddy did you change bits in the example above?

In any case, I'm for it (without the 0x though), but it would break code.

mruddy commented at 1:58 AM on March 31, 2016: contributor

@dcousens no, but that was from a node running -regtest.

Yes, for that reason I'll probably put up a different commit that just has another field for the hex encoding. I think I'll stick with the 0x because then it's visually obvious that it's not base 10. I'm thinking about only including this new field for versions other than 1-4, but I haven't decided yet.

dcousens commented at 2:03 AM on March 31, 2016: contributor

Maybe versionBits: 'ffff0000' instead of replacing version

mruddy commented at 2:24 AM on March 31, 2016: contributor

That sounds pretty good although I'm hesitant to use the terminology that overlaps with BIP9. For example, there are two more possible future mechanisms where a hex encoding might also be helpful (and I think some BIP101 blocks used something very close or similar to BIP9 IIRC). So, how about versionHex: 'ffff0000'. That is obvious enough to drop the 0x and still generic enough that I don't have to apply BIP specific logic to it or create another field in the far future. The only decision is whether to show this field for versions 1-4. I'm leaning towards not showing it for versions 1-4.

dcousens commented at 2:33 AM on March 31, 2016: contributor

versionHex: 'ffff0000' :+1:

mruddy force-pushed on Mar 31, 2016

mruddy commented at 3:16 AM on March 31, 2016: contributor

Cool, yeah that works for me. Thanks for the help!

mruddy renamed this:
~~RPC: BIP9 version bits related, format version as hex in getblock and getblockheader~~
RPC: add versionHex in getblock and getblockheader JSON results
on Mar 31, 2016

luke-jr commented at 4:55 AM on March 31, 2016: member

RPC isn't meant for human consumption. Do this in the client.

laanwj commented at 9:05 AM on March 31, 2016: member

I agree with @luke-jr. This is a number, keep passing it as a number. Client software can format it to the user however they want. (we do this consistently in other places too; e.g. we even report timestamps as numbers, and don't bother formatting them as date-times)

It's certainly not worth breaking the interface for. Also realize that a client may need this value to process further, not to show to the user: e.g. bitwise-AND operations should be done on a number, not a string, so this change may mean doing an extra conversion from hex to int on the client side.

No strong opposition to adding it as extra field, though I'd still say adding a field to avoid an extra hex conversion on the client side is too much hand-holding. This shouldn't set a precedent to add hex, octal, binary for every field.

laanwj added the label RPC/REST/ZMQ on Mar 31, 2016

in src/rpc/blockchain.cpp:None in 500f516f11 outdated

  69 | @@ -70,6 +70,10 @@ UniValue blockheaderToJSON(const CBlockIndex* blockindex)
  70 |      result.push_back(Pair("confirmations", confirmations));
  71 |      result.push_back(Pair("height", blockindex->nHeight));
  72 |      result.push_back(Pair("version", blockindex->nVersion));
  73 | +    if (blockindex->nVersion < 1 || blockindex->nVersion > 4)

laanwj commented at 9:10 AM on March 31, 2016:

do we have these 1 and 4 available as constants somewhere?

Edit: or maybe just always add it

mruddy force-pushed on Mar 31, 2016

mruddy commented at 12:18 PM on March 31, 2016: contributor

@laanwj Updated to use the VERSIONBITS_LAST_OLD_BLOCK_VERSION constant and added help text. I opted to not always add the new hex formatted field to avoid redundancy when the version is visually obvious.

laanwj commented at 9:40 AM on April 2, 2016: member

BTW we should probably add the bit number to bip9_softforks in getblockchaininfo. It would provide some context to these bits. (doesn't necessarily have to be in this pull, but it's another idea to make BIP9 more visible and transparent, and allowing clients to interpret this)

paveljanik commented at 9:50 AM on April 2, 2016: contributor

Or add some command to list the defined bip9 softforks with their starttime, timeout and bit.

laanwj commented at 10:21 AM on April 2, 2016: member

Well that can all be on getblockchaininfo IMO.

More on topic: another wild idea here would be to try to actually decode the versionbits and return a list of deployment names. That is at least non-trivial handholding as they are context dependent.

btcdrak commented at 11:23 AM on April 2, 2016: contributor

@laanwj You could only decode the BIP9 deployments your version understands, so earlier versions are just going to return "unknown" and it's also conceivable that Bitcoin Core would not necessarily have the definitions for all softforks (because it doesnt support them). For a full "decode", we should better rely on a file BIPs which lists all known deployments, and it would also serve to help the community co-ordinate on free bits.

mruddy force-pushed on Apr 2, 2016

mruddy commented at 2:01 PM on April 2, 2016: contributor

I went ahead and added the extra data to bip9_softforks in getblockchaininfo.

The first thing to decide was if/when to show the bit information. Since the relevant bit information for a given block version changes over time, it would be confusing to always show the bit info for every soft fork that was ever defined with this mechanism. It's actually not quite as straightforward as I guessed it would be. For example, BIP9 gives the guidance:

"Miners should continue setting the bit in LOCKED_IN phase so uptake is visible, though this has no effect on consensus rules."

"There could be two non-overlapping deployments on the same bit, where the first one transitions to LOCKED_IN while the other one simultaneously transitions to STARTED, which would mean both would demand setting the bit."

"Once the consensus change succeeds or times out, there is a "fallow" pause after which the bit can be reused for later changes."

Although the plan is to have a fallow period, due to consensus allowing a locked in bit to be immediately reused, it might not really be safe for a miner to continue setting the bit. EDIT: this is also feedback to @sipa about ComputeBlockVersion continuing to set the version bit during LOCKED_IN, assuming my understanding of BIP9 is correct. Thus, it might not make sense to show the info for a locked in soft fork even though during normal operations, the fallow period is observed and it would make sense to give a user the info.

So, I took the conservative approach and decided to only show the bit info during an active voting period, that is when the soft fork is in state STARTED. This gives an indication to users that if the hex block version is other than 20000000 and no info is shown in bip9_softforks, then either everything is locked in, or there is fork voting going on for something the node software being used does not know about.

I left the condition as a separate block in-case we decide to change it to THRESHOLD_STARTED == thresholdState || THRESHOLD_LOCKED_IN == thresholdState. I figured I'd throw this out there and see what other people thought.

mruddy force-pushed on Apr 2, 2016

laanwj commented at 12:05 PM on April 3, 2016: member

it's also conceivable that Bitcoin Core would not necessarily have the definitions for all softforks (because it doesnt support them) @btcdrak I agree. It can't be complete, but it can show the known ones. Tons of options for the unknown ones, one would be to show them as 'unknownXX' where XX is the bit. E.g. in UpdateTip logging we issue a warning per block, see #7795.

IMO where to coordinate the bits is completely offtopic here :) It shouldn't happen in our repository at all, but indeed in BIPs. @mruddy Sounds good to me.

btcdrak commented at 5:09 PM on April 3, 2016: contributor

@laanwj Yes. I like the patch as it is now.

RPC: add versionHex in getblock and getblockheader JSON results; expand data in getblockchaininfo bip9_softforks field. 92107d574d

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

 323 | @@ -316,6 +324,7 @@ UniValue getblockheader(const UniValue& params, bool fHelp)
 324 |              "  \"confirmations\" : n,   (numeric) The number of confirmations, or -1 if the block is not on the main chain\n"
 325 |              "  \"height\" : n,          (numeric) The block height or index\n"
 326 |              "  \"version\" : n,         (numeric) The block version\n"
 327 | +            + strprintf("  \"versionHex\" : \"00000000\", (string) The block version formatted in hexadecimal when version is greater than %d\n", VERSIONBITS_LAST_OLD_BLOCK_VERSION) +

laanwj commented at 7:24 AM on April 4, 2016:

"when version is greater than %d" looks kind of arbitrary, out of context. Maybe we should mention BIP9 explicitly in the help. The reason for this threshold is that BIP9 version bits start to be relevant, right?

mruddy commented at 11:39 AM on April 4, 2016:

Kind of. It's a little more generic than that though. That threshold is the highest version number that I know for sure will be treated as a simple integer. I wanted to avoid redundancy for the easy to ascertain version numbers that are definitely treated as integers.

I was trying to avoid being too specific to BIP9 because it allows for two future mechanisms (top bits 010 and 011). I thought of using >= VERSIONBITS_TOP_BITS, but that ignores [5, 0x1fffffff] which I think could be re-purposed too. I don't see any rule limiting that range's use to only being simple integers.

I should probably just keep it simple and always display versionHex. My redundancy limiting kinda feels like unnecessary optimization.

mruddy force-pushed on Apr 4, 2016

mruddy commented at 10:40 PM on April 4, 2016: contributor

ok, i pushed up a commit that simplifies and always shows the versionHex for getblock and getblockheader instead of only showing it when version > 4.

laanwj merged this on Apr 5, 2016

laanwj closed this on Apr 5, 2016

laanwj referenced this in commit 916b15a87a on Apr 5, 2016

btcdrak commented at 2:10 PM on April 5, 2016: contributor

post humus ACK

laanwj commented at 2:10 PM on April 5, 2016: member

Tested ACK 92107d5

mruddy deleted the branch on Apr 5, 2016

in src/rpc/blockchain.cpp:None in 92107d574d

 663 | @@ -653,6 +664,9 @@ UniValue getblockchaininfo(const UniValue& params, bool fHelp)
 664 |              "     {\n"
 665 |              "        \"id\": \"xxxx\",        (string) name of the softfork\n"
 666 |              "        \"status\": \"xxxx\",    (string) one of \"defined\", \"started\", \"lockedin\", \"active\", \"failed\"\n"
 667 | +            "        \"bit\": xx,             (numeric) the bit, 0-28, in the block version field used to signal this soft fork\n"

MarcoFalke commented at 1:13 PM on April 9, 2016:

Nit: Prob good to mention this only shows up when status=="started"

mruddy commented at 4:07 PM on April 9, 2016:

that had crossed my mind before. either way works for me.

in qa/rpc-tests/blockchain.py:None in 92107d574d

  81 | @@ -82,6 +82,7 @@ def _test_getblockheader(self):
  82 |          assert isinstance(header['mediantime'], int)
  83 |          assert isinstance(header['nonce'], int)
  84 |          assert isinstance(header['version'], int)
  85 | +        assert isinstance(int(header['versionHex'], 16), int)

MarcoFalke commented at 1:15 PM on April 9, 2016:

Nit: assert_is_hex_string is enough because an int is always instance of int.

mruddy commented at 4:06 PM on April 9, 2016:

yep, that'd be a little cleaner

sickpig referenced this in commit 3c6d13ab50 on Mar 31, 2017

codablock referenced this in commit d8ac38fe4e on Sep 16, 2017

codablock referenced this in commit e653ac9a6c on Sep 19, 2017

codablock referenced this in commit 2137356c60 on Dec 9, 2017

codablock referenced this in commit 0591eef8dd on Dec 19, 2017

codablock referenced this in commit e7d9ffa5d7 on Dec 19, 2017

MarcoFalke locked this on Sep 8, 2021