Skip to content

Fix help, add RPC tests for getblockheader#7194

Merged
laanwj merged 3 commits intobitcoin:masterfrom
jamesob:test_getblockheader
Jan 18, 2016
Merged

Fix help, add RPC tests for getblockheader#7194
laanwj merged 3 commits intobitcoin:masterfrom
jamesob:test_getblockheader

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

jonasschnelli commented Dec 10, 2015

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 Outdated
Copy link
Member

@laanwj laanwj Dec 10, 2015

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

@jamesob jamesob Dec 10, 2015

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

@petertodd petertodd Dec 11, 2015

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

@jamesob jamesob Dec 11, 2015

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

petertodd commented Dec 11, 2015

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 Outdated
Copy link
Member

@laanwj laanwj Dec 14, 2015

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

@jamesob jamesob Dec 14, 2015

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

@jamesob jamesob Dec 15, 2015

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

@instagibbs instagibbs Dec 18, 2015

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

@instagibbs instagibbs Dec 18, 2015

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

@jamesob jamesob Jan 4, 2016

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

@instagibbs instagibbs Jan 4, 2016

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

@jamesob jamesob Jan 4, 2016

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

@instagibbs instagibbs Jan 4, 2016

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