Add complete set of RPC Calls (as of 0.19.0.1) #242
Conversation
bitcoin/tests/test_rpc.py
Outdated
| # pass | ||
|
|
||
| def test_gettxout(self): | ||
| """Txout disappears if spent, so difficult to set static test""" |
There was a problem hiding this comment.
This test will break often. As long as the txout exists, the test works though. See comment.
| block1 = proxy.getblock(blockhash) | ||
| self.assertTrue(isinstance(block1, CBlock)) | ||
| # Test from str | ||
| block2 = proxy.getblock("0000000000000000000b4b0daf89eac9d84138fc900b8c473d4da70742e93dd0") |
There was a problem hiding this comment.
This assumes that the daemon connected to by default tracks Bitcoin mainnet. This is totally not guaranteed.
There was a problem hiding this comment.
And in the automatic test environment spawning a full mainnet node will be too expensive.
There was a problem hiding this comment.
indeed. what are you proposing?
There was a problem hiding this comment.
special env var + regtest node, see my other comment, on test_getblockcount
|
|
||
| def test_getblockcount_et_al(self): | ||
| # This test could possibly false-fail if new blocks arrive. | ||
| # Highly unlikely since they're quick calls |
There was a problem hiding this comment.
I don't think "highly unlikely" is a good argument. It will someday fail and confuse someone. Tests should be deterministic.
By the way, because the tests should be deterministic, I don't think that just connecting to with the default parameters is a good idea - the tests will depend on environment that they run in, and what bitcoin.conf is there is unpredictable. I would introduce a special environment variable with unique prefix, like BITCOINLIB_TEST_RUN_ENABLE_RPC_CONF=...some_test_bitcoin.conf or something like that, that would indicate that the environment is ready for the tests, and should use a special config. This way an automatic test service like travis ci can be used to setup a test environment where RPC can be tested, but the tests won't have any unexpected effects in other environments.
In general, user would not expect that some tests could even connect to their working bitcoind, for example, and may not be happy about that even if the commands are innocuous. On the other hand, the connection might block if the daemon port is filtered via dropping the packets rather than sending RST back, and then the test will be stuck for quite a long time.
There was a problem hiding this comment.
Sorry, this is confusing for me. Are you saying we should add a separate bitcoin.conf just for when a user runs rpc tests? I'm not sure there's a failsafe way to test getblockcount. Do you have any ideas?
There was a problem hiding this comment.
I propose to use a specially-named environment variable to point to a separate bitcoin.conf that would contain the parameters for the test. If an environment variable is not specified, the RPC tests just not run. If it is specified, a failed connection means error.
You can safely test blockcount on a regtest node. As I said in another comment, relying on a mainnet node will be unrealistic if you want your tests to be run in some automatic test system like Travis CI - your test container would need to include a full node into it. Even if you manage that, a full node that connects to the network is a dynamic thing that won't be cached and your test times will be very long
There was a problem hiding this comment.
how do you set the chain with a cli command? Is this possible?
There was a problem hiding this comment.
What do you think about just removing this test? getblockcount seems easy enough to get right.
There was a problem hiding this comment.
I propose to use a specially-named environment variable to point to a separate bitcoin.conf that would contain the parameters for the test. If an environment variable is not specified, the RPC tests just not run. If it is specified, a failed connection means error.
bitcoind's test_framework has some functionality for this out of the box. Should probably be packaged into a python library at some point.
There was a problem hiding this comment.
The idea is that you would prepare your test environment in a way that a regtest bitcoind is already running and a special bitcoin.conf already points on its rpc. you indicate that with a special env var. You won't need to set the chain with a command. This is workable when you have automatic testing in a container. When manually running the tests, you will usually just not set that special env var and will skip the rpc tests.
What do you think about just removing this test?
I have no opinion - I'm not sure that these tests along with the functionality they test for are needed at all, and I removed that functionality from bitcointx entirely in favour of a dumb RPC proxy without converting any values. Others might hold different approach to this, and that's OK. But I saw a few potential problems with these tests, that I thought are worth highlighting.
There was a problem hiding this comment.
Okay. I am not very experienced with writing tests, so I'm getting a bit lost here. @kanzure, by your comment, did you mean there's a tool I can use to facilitate this?
There was a problem hiding this comment.
https://github.com/bitcoin/bitcoin/tree/master/test/functional/test_framework
You don't have to use this; porting it over to python-bitcoinlib would be a bit of a project. But I wanted to point out that it exists and helps with testing (setting up multiple nodes, wiring them up, etc).
TLDR: I added all RPC calls to bitcoin/rpc.py and slightly altered some existing calls to update the API. I also added OP_TRUE and OP_FALSE to script.py.
Changes:
I added all RPC calls listed in v0.19.0.1 and reordered all the functions so that their order matches the order provided in
bitcoin-cli help. This also sorts all calls roughly by their function. Additionally, I added the ability for calls to accept strings or direct JSON, which I thought made several functions much more flexible. Many comments I made while writing the code remain, but if this is undesirable I can remove them.Testing:
I added a few tests to bitcoin/tests/test_rpc.py, but all calls require a running Core instance and testing many calls would be inconvenient. I manually tested almost every function in rpc.py, with the exception of those which timed out and the mining or PSBT-related calls. I ran all existing unittests and those I created with perfect success (with a running Core instance). Some RPC testing relies on transactions being in the UTXO set, so if they are spent, a new UTXO must be found.
I did my best to ensure every function works in a backwards-compatible way and the previous style was continued to the best of my ability. For this reason, I added duplicate functions for the call
sendrawtransactionandfundrawtransaction(named sendrawtransactionv0_19 and fundrawtransactionv0_19). Eventually, these new functions should replace the old.Future Work:
I declined to change the API for
sendmanyandsendtoaddressbecause they were substantially different. This should be fixed (I will do this later).Other deprecated calls (such as generate and getinfo) should also be removed in due time.
Proxy calls will timeout after 30 seconds, while several Core RPC calls usually take longer. These calls are thus useless for now.
The
stopcall also does not work, so it is commented out for now.New RPC calls should be added to the examples folder, along with more comprehensive testing.