Fix help, add RPC tests for getblockheader #7194

pull jamesob wants to merge 3 commits into bitcoin:master from jamesob:test_getblockheader changing 3 files +54 −2

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:None 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:None 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:None 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:

    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:None 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](/bitcoin-bitcoin/contributor/petertodd/). 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

Contributors

Labels

Docs Tests RPC/REST/ZMQ

Linked (view graph)

#7373 [Tests] Fix error in blockchain.py introduced in merge