Skip to content
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.

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

Bitcoin Cash implementations/compatibility #498

Closed
ghost opened this issue Oct 5, 2020 · 11 comments
Closed

Bitcoin Cash implementations/compatibility #498

ghost opened this issue Oct 5, 2020 · 11 comments

Comments

@ghost
Copy link

ghost commented Oct 5, 2020

Hello,
It's more a question than a bug report: I have a machine set up with Bitcoin Cash ABC and Blockbook. This way it works fine with the following config.json:

{
    "coin_name": "Bcash",
    "coin_shortcut": "BCH",
    "coin_label": "Bitcoin Cash",
    "rpc_url": "****",
    "rpc_user": "****",
    "rpc_pass": "****",
    "rpc_timeout": 25,
    "parse": true,
    "message_queue_binding": "****",
    "subversion": "",
    "address_format": "",
    "block_addresses_to_keep": 300
}

Now I need to test it with other implementations, like Bitcoin Cash Node or BCHD. With both of these I got an error message from Blockbook. E.g. for BCHD it says:

E1003 15:29:52.260269       1 blockbook.go:371] rpc: unexpected end of JSON input Retrying...
E1003 15:29:53.260388       1 blockbook.go:371] rpc: unexpected end of JSON input Retrying...
E1003 15:29:54.260436       1 blockbook.go:371] rpc: unexpected end of JSON input Retrying...

Is Blockbook compatible with any other implementation than ABC? If so which one or how shall I change the config for it to work?

@martinboehm
Copy link
Contributor

Hi, we have not implemented/tested Blockbook with any other node than Bitcoin Cash ABC. From your report it seems that the rpc protocol differs in other implementations. I would expect that the difference will not be big but still need to be addressed in the code.

You are welcome to investigate it and create a PR for it.

@quietnan
Copy link

This change is at fault for Bitcoin Cash Node: https://gitlab.com/bitcoin-cash-node/bitcoin-cash-node/-/commit/5eb88756f3a787a9e57464181df785b46d72a2b3

Renaming blockhash to hash_or_height. Quickfix renaming works for Bitcoin Cash Node, obviously breaks all other BTC-ish currencies.
So far for the 'investigate' part. PR might follow if I find the time, but I can't promise it.

@sickpig
Copy link

sickpig commented Oct 13, 2020

hash_or_height is the same param name chosen by Core when they decided to make getblockstat works both with block height and hash.

We at BCHN and BCH Unlimited (in short BU) simply extended the application of the concept and used it also for getblock (BCHN and BU) and getheader (BU only).

Another possible fix, on top of the ones mentioned by @quietnan, is to call the RPC w/o using named parameter. Dunno if it is possible thou.

Will have a look at blockbook codebase.

@ftrader
Copy link

ftrader commented Oct 13, 2020

@quietnan : Thanks for your investigation.

It looks like it might be possible to add a blockhash argument name alias in BCHN to restore compatibility with ABC there, but I would caution against relying only on ABC node as a reference as they don't seem to have the support of the majority of the BCH network for the coming upgrade. (hashpower, number of nodes, fork future prices)

We will add more information about upstream resolution in BCHN here as it becomes available.

@cculianu
Copy link

@quietnan We have already created a fix and are reviewing it. We anticipate this fix appearing in the next version of BCHN -- https://gitlab.com/bitcoin-cash-node/bitcoin-cash-node/-/merge_requests/810

ftrader pushed a commit to bitcoin-cash-node/bitcoin-cash-node that referenced this issue Oct 16, 2020
…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.
@ftrader
Copy link

ftrader commented Oct 16, 2020

@quietnan @martinboehm @g574

BCHN release v22.1.0 is out, and include a compatibility fix which restores the 'blockhash' argument name (as an alias).

https://github.com/bitcoin-cash-node/bitcoin-cash-node/releases/tag/v22.1.0

So if it works with ABC, it should work with BCHN v22.1.0 too.

Looking forward to hearing if this solves the issue you were experiencing.

@martinboehm
Copy link
Contributor

@quietnan If we wanted to test it, are the ABC and BCHN blockchain databases binary compatible (ie. it is enough to use BCHN binaries with formerly ABC database) or is it necessary to resync it from start?

@ftrader
Copy link

ftrader commented Oct 16, 2020

are the ABC and BCHN blockchain databases binary compatible (ie. it is enough to use BCHN binaries with formerly ABC database)

Yes, they are still compatible, @martinboehm

@cculianu
Copy link

cculianu commented Oct 16, 2020

100% the exact same format .. just swap the binaries to test and you can use the same CONFIG files (and data dir) and should work 100% @martinboehm

@ghost
Copy link
Author

ghost commented Oct 19, 2020

@cculianu, @ftrader, @martinboehm, thank you for help. Bitcoin Cash Node seems to work with Blockbook now. I'll get back to you if I run into any issues.

@ghost ghost closed this as completed Oct 19, 2020
@cculianu
Copy link

@g574 Much appreciated. If you ever have bugs that you think are Bitcoin Cash Node's fault again, feel free to write in this thread, or contact us directly on our gitlab.com repo: https://gitlab.com/bitcoin-cash-node/bitcoin-cash-node <-- creating an issued here will work -- we respond quickly.

Our website also has a link to our slack -- we are always on slack!: https://bitcoincashnode.org/

This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants