Fix help, add RPC tests for getblockheader

jamesob commented at 5:06 pm on December 9, 2015: member

Continuing to chip away at the untested RPC interface. This PR:

adds tests for getblockheader (previously uncovered)
adds missing documentation to the getblockheader RPC help
adds two assertion utilities to the RPC testing framework

jonasschnelli added the label Docs and Output on Dec 10, 2015

jonasschnelli added the label Tests on Dec 10, 2015

jonasschnelli added the label RPC on Dec 10, 2015

jonasschnelli commented at 7:40 am on December 10, 2015: contributor

Nice. utACK. tiny nit: don’t label rpcblockchain.cpp changes (help output) as [test]. Maybe better open a 2nd PR.

in qa/rpc-tests/test_framework/util.py: in 82088e2d0e outdated

406@@ -407,5 +407,22 @@ def assert_raises(exc, fun, *args, **kwds):
407     else:
408         raise AssertionError("No exception raised")
409 
410+def assert_is_hex_string(string):
411+    try:
412+        int(string, 16)
413+    except Exception as e:
414+        raise AssertionError(
415+            "Couldn't interpret %r as hexidecimal; raised: %s" % (string, e))

laanwj commented at 11:33 am on December 10, 2015:

hexadecimal

jamesob commented at 4:25 pm on December 10, 2015:

fixed

jamesob force-pushed on Dec 10, 2015

jamesob renamed this:
~~[tests] Add RPC tests for getblockheader~~
Fix help, add RPC tests for getblockheader
on Dec 10, 2015

jamesob commented at 4:26 pm on December 10, 2015: member

@jonasschnelli good point, thanks. I’ve changed the PR title to reflect this.

morcos commented at 5:40 pm on December 10, 2015: member

utACK ~~modulo question~~

jamesob commented at 6:22 pm on December 10, 2015: member

talked with @morcos out-of-band, sounds as though he’s cool with changes as they are.

in src/rpcblockchain.cpp: in 3af5edccd0 outdated

322@@ -323,7 +323,8 @@ UniValue getblockheader(const UniValue& params, bool fHelp)
323             "  \"bits\" : \"1d00ffff\", (string) The bits\n"
324             "  \"difficulty\" : x.xxx,  (numeric) The difficulty\n"
325             "  \"previousblockhash\" : \"hash\",  (string) The hash of the previous block\n"
326-            "  \"nextblockhash\" : \"hash\"       (string) The hash of the next block\n"
327+            "  \"nextblockhash\" : \"hash\",      (string) The hash of the next block\n"
328+            "  \"chainwork\" : \"0000...1f3\"     (string) Number of hashes expected to produce the current chain (in hex)\n"

petertodd commented at 6:35 am on December 11, 2015:

What about “Expected number of hashes required to produce the current chain”?

Current wording could be read as “do that work, will get the current chain”, which is a bit unlikely. :)

jamesob commented at 4:37 pm on December 11, 2015:

sure thing. fixed

petertodd commented at 6:41 am on December 11, 2015: contributor

utACK modulo wording

jamesob force-pushed on Dec 11, 2015

Add assert_is_hex_string and assert_is_hash_string to RPC test utils. 16d4fce0b2

Add RPC documentation for getblockheader[chainwork]. 4745636126

Add RPC tests for getblockheader. 135d6ec8ce

in qa/rpc-tests/test_framework/util.py: in 442afca81c outdated

418+    if not isinstance(string, basestring):
419+        raise AssertionError("Expected a string, got type %r" % type(string))
420+    elif length and len(string) != length:
421+        raise AssertionError(
422+            "String of length %d expected; got %d" % (length, len(string)))
423+    elif not re.match('[abcdef0-9]+', string):

laanwj commented at 12:54 pm on December 14, 2015:

I think you want:

0    elif not re.match('[abcdef0-9]+$', string):

jamesob commented at 6:38 pm on December 14, 2015:

Very true :). willfix

jamesob commented at 3:23 pm on December 15, 2015:

fixed

jamesob force-pushed on Dec 14, 2015

in src/rpcblockchain.cpp: in 135d6ec8ce

322@@ -323,7 +323,8 @@ UniValue getblockheader(const UniValue& params, bool fHelp)
323             "  \"bits\" : \"1d00ffff\", (string) The bits\n"
324             "  \"difficulty\" : x.xxx,  (numeric) The difficulty\n"
325             "  \"previousblockhash\" : \"hash\",  (string) The hash of the previous block\n"
326-            "  \"nextblockhash\" : \"hash\"       (string) The hash of the next block\n"
327+            "  \"nextblockhash\" : \"hash\",      (string) The hash of the next block\n"
328+            "  \"chainwork\" : \"0000...1f3\"     (string) Expected number of hashes required to produce the current chain (in hex)\n"

instagibbs commented at 6:57 pm on December 18, 2015:

It’s the expected number of hashes up to this point in the chain, not the entire chain. AFAIK

instagibbs commented at 7:33 pm on December 18, 2015:

I added a similar line for getblock at #7232

jamesob commented at 6:02 pm on January 4, 2016:

This message is based on @sipa’s description of chainwork and [was vetted previously by @petertodd](https://github.com/bitcoin/bitcoin/pull/7194#discussion_r47325775). You may want to close the PR you reference above; it was opened after this one and doesn’t include tests.

instagibbs commented at 6:13 pm on January 4, 2016:

Ok, I disagree that it’s clear, but if everyone else is fine with it I can conform as well.

Regardless, my pull is not covered here, so not sure why would I close it?

edit: My title wasn’t clear, so I added the specific rpc.

jamesob commented at 6:17 pm on January 4, 2016:

@instagibbs thanks!

instagibbs commented at 9:23 pm on January 4, 2016:

Perhaps my newer suggestion is equitable?

https://github.com/bitcoin/bitcoin/pull/7232/files#r48769375

jamesob commented at 7:27 pm on December 29, 2015: member

ping on this

sipa commented at 7:29 pm on December 29, 2015: member

I’ll start reviewing again after new year.

laanwj commented at 11:21 am on January 18, 2016: member

utACK

laanwj merged this on Jan 18, 2016

laanwj closed this on Jan 18, 2016

laanwj referenced this in commit e4e77ee55d on Jan 18, 2016

laanwj referenced this in commit 351ffd8482 on Jan 18, 2016

MarcoFalke locked this on Sep 8, 2021

Fix help, add RPC tests for getblockheader #7194