Skip to content

Fix help, add RPC tests for getblockheader #7194

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 3 commits into from
Jan 18, 2016
Merged

Fix help, add RPC tests for getblockheader #7194

merged 3 commits into from
Jan 18, 2016
+54 −2

Conversation

jamesob
Copy link
Contributor

@jamesob jamesob commented Dec 9, 2015

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
Copy link
Contributor

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

laanwj
laanwj reviewed Dec 10, 2015
View reviewed changes
qa/rpc-tests/test_framework/util.py
int(string, 16)
except Exception as e:
raise AssertionError(
"Couldn't interpret %r as hexidecimal; raised: %s" % (string, e))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hexadecimal

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

@jamesob jamesob force-pushed the test_getblockheader branch from 82088e2 to 3af5edc Compare December 10, 2015 16:24
@jamesob jamesob changed the title [tests] Add RPC tests for getblockheader Fix help, add RPC tests for getblockheader Dec 10, 2015
@jamesob
Copy link
Contributor Author

jamesob commented Dec 10, 2015

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

@morcos
Copy link
Contributor

morcos commented Dec 10, 2015

utACK modulo question

@jamesob
Copy link
Contributor Author

jamesob commented Dec 10, 2015

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

petertodd
petertodd reviewed Dec 11, 2015
View reviewed changes
src/rpcblockchain.cpp
@@ -323,7 +323,8 @@ UniValue getblockheader(const UniValue& params, bool fHelp)
" \"bits\" : \"1d00ffff\", (string) The bits\n"
" \"difficulty\" : x.xxx, (numeric) The difficulty\n"
" \"previousblockhash\" : \"hash\", (string) The hash of the previous block\n"
" \"nextblockhash\" : \"hash\" (string) The hash of the next block\n"
" \"nextblockhash\" : \"hash\", (string) The hash of the next block\n"
" \"chainwork\" : \"0000...1f3\" (string) Number of hashes expected to produce the current chain (in hex)\n"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sure thing. fixed

@petertodd
Copy link
Contributor

utACK modulo wording

@jamesob jamesob force-pushed the test_getblockheader branch from 3af5edc to 442afca Compare December 11, 2015 16:37
laanwj
laanwj reviewed Dec 14, 2015
View reviewed changes
qa/rpc-tests/test_framework/util.py
elif length and len(string) != length:
raise AssertionError(
"String of length %d expected; got %d" % (length, len(string)))
elif not re.match('[abcdef0-9]+', string):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you want:

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very true :). willfix

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

@jamesob jamesob force-pushed the test_getblockheader branch from 442afca to 135d6ec Compare December 14, 2015 18:43
instagibbs
instagibbs reviewed Dec 18, 2015
View reviewed changes
src/rpcblockchain.cpp
@@ -323,7 +323,8 @@ UniValue getblockheader(const UniValue& params, bool fHelp)
" \"bits\" : \"1d00ffff\", (string) The bits\n"
" \"difficulty\" : x.xxx, (numeric) The difficulty\n"
" \"previousblockhash\" : \"hash\", (string) The hash of the previous block\n"
" \"nextblockhash\" : \"hash\" (string) The hash of the next block\n"
" \"nextblockhash\" : \"hash\", (string) The hash of the next block\n"
" \"chainwork\" : \"0000...1f3\" (string) Expected number of hashes required to produce the current chain (in hex)\n"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added a similar line for getblock at #7232

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This message is based on @sipa's description of chainwork and was vetted previously by @petertodd. You may want to close the PR you reference above; it was opened after this one and doesn't include tests.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@instagibbs thanks!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps my newer suggestion is equitable?

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

@jamesob
Copy link
Contributor Author

jamesob commented Dec 29, 2015

ping on this

@sipa
Copy link
Member

sipa commented Dec 29, 2015 via email

@laanwj
Copy link
Member

laanwj commented Jan 18, 2016

utACK

@laanwj laanwj merged commit 135d6ec into bitcoin:master Jan 18, 2016
laanwj added a commit that referenced this pull request Jan 18, 2016
@laanwj
Merge pull request #7194
e4e77ee 
135d6ec Add RPC tests for getblockheader. (James O'Beirne)
4745636 Add RPC documentation for getblockheader[chainwork]. (James O'Beirne)
16d4fce Add assert_is_hex_string and assert_is_hash_string to RPC test utils. (James O'Beirne)
laanwj pushed a commit that referenced this pull request Jan 18, 2016
@jamesob @laanwj
Fix help, add RPC tests for getblockheader
351ffd8 
- Add assert_is_hex_string and assert_is_hash_string to RPC test utils.
- Add RPC documentation for getblockheader[chainwork].
- Add RPC tests for getblockheader.

Github-Pull: #7194
Rebased-From: 16d4fce 4745636 135d6ec
@sdaftuar sdaftuar mentioned this pull request Jan 18, 2016
Merged
@instagibbs instagibbs mentioned this pull request Apr 27, 2016
Closed
7 tasks
@dagurval dagurval mentioned this pull request Dec 18, 2017
Merged
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
No reviews
Assignees
No one assigned
Labels
Docs RPC/REST/ZMQ Tests
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@jamesob @jonasschnelli @morcos @petertodd @sipa @laanwj @instagibbs