Skip to content

Commit

Permalink
RPC: Add alias blockhash to getblockheader for backward compatibi…
Browse files Browse the repository at this point in the history
…lity

Summary
----

It was found that recent changes to our RPC API introduced here: https://gitlab.com/bitcoin-cash-node/bitcoin-cash-node/-/merge_requests/443
caused the named parameter "blockhash" to be renamed to
"hash_or_height".  It turns out some extant software in the wild relied
on this named parameter to not change.  See: trezor/blockbook#498 (comment)

Fortunately, this codebase supports named parameter aliasing by using
the '|' character inside the parameter names in the RPC method table.
So, we leverage this facility and make `getblockheader` also accept
`blockhash` interchangeably with the `hash_or_height` named parameter.
This ensures continued 100% RPC compatibility with ABC and older BCHN.

The rpc_blockchain unit test for `getblockheader` was also
updated/modified to test that this alias continues to work.

Closes Bitcoin-ABC#173 .

Test Plan
---

This should cover it:

- `ninja && test/functional/test_runner rpc_blockchain`

Also manual testing to verify in case you are paranoid, do the below
with and without this commit (testnet3 assumed below):

`curl --user USER_NAME_HERE --data-binary '{"jsonrpc": "1.0", "id":"curltest", "method": "getblockheader", "params": {"blockhash":"000000008a91ac6ac787eda6a2f5a7ddc94311322c3f80ddf0f144cb7c2ba7bc"} }' -H 'content-type: text/plain;' http://127.0.0.1:18332/`

- Without this commit: should complain about unknown named arg `blockhash`
- With this commit: should accept arg and return a header for block

Note that you may need to adjust the above username and port for your
setup. The above example assumes testnet3.
  • Loading branch information
cculianu committed Oct 13, 2020
1 parent db686fa commit 897b181
Show file tree
Hide file tree
Showing 2 changed files with 17 additions and 1 deletion.
2 changes: 1 addition & 1 deletion src/rpc/blockchain.cpp
Expand Up @@ -2563,7 +2563,7 @@ static const ContextFreeRPCCommand commands[] = {
{ "blockchain", "getblockchaininfo", getblockchaininfo, {} },
{ "blockchain", "getblockcount", getblockcount, {} },
{ "blockchain", "getblockhash", getblockhash, {"height"} },
{ "blockchain", "getblockheader", getblockheader, {"hash_or_height","verbose"} },
{ "blockchain", "getblockheader", getblockheader, {"blockhash|hash_or_height","verbose"} },
{ "blockchain", "getblockstats", getblockstats, {"hash_or_height","stats"} },
{ "blockchain", "getchaintips", getchaintips, {} },
{ "blockchain", "getchaintxstats", getchaintxstats, {"nblocks", "blockhash"} },
Expand Down
16 changes: 16 additions & 0 deletions test/functional/rpc_blockchain.py
Expand Up @@ -284,6 +284,22 @@ def _test_getblockheader(self):
header_by_height = node.getblockheader(hash_or_height=200)
assert_equal(header, header_by_height)

# Next, check that the old alias 'blockhash' still works
# and is interchangeable with hash_or_height
# First, make sure errors work as expected for unknown named params
self.log.info("Testing that getblockheader(blockhashhh=\"HEX\") produces the proper error")
assert_raises_rpc_error(
-8,
"Unknown named parameter blockhashhh",
node.getblockheader,
blockhashhh=header['hash'])
# Next, actually try the old legacy blockhash="xx" style arg
self.log.info("Testing that legacy getblockheader(blockhash=\"HEX\") still works ok")
header_by_hash2 = node.getblockheader(blockhash=header['hash'])
assert_equal(header, header_by_hash2)
header_by_height2 = node.getblockheader(blockhash=200)
assert_equal(header, header_by_height2)

# check that we actually get a hex string back from getblockheader
# if verbose is set to false.
header_verbose_false = node.getblockheader(200, False)
Expand Down

0 comments on commit 897b181

Please sign in to comment.