[WIP] Adds support for `NetworkAPI` to use a remote Bitcoin node #76

bjarnemagnussen · 2019-05-25T16:32:31Z

This PR adds a method allowing the NetworkAPI to connect to a remote Bitcoin node instead of relying on web APIs. It is marked as a work-in-progress to allow testing, code improvements and test cases to be added.

It has been implemented in a way to minimize changes to the codebase and to not break already existing tests. Any suggestions however on how to better connect the new class bit.network.services.RPCHost with the NetworkAPI is welcome! Right now inside NetworkAPI it simply overrides its web-API values using the RPCHost instead.

This implementation also allows to connect to two different Bitcoin nodes: one for mainnet and one for testnet; and keeps them separate.

Example code

import bit

# To connect to a remote Bitcoin node:
from bit.network import NetworkAPI
NetworkAPI.connect_to_node(user='user', password='password', host='localhost', port='18443', use_https=False, testnet=True)

# We could additionally choose to connect to some mainnet node:
NetworkAPI.connect_to_node(user='user', password='password', host='domain', port='8332', use_https=True, testnet=False)

# Do some bit stuff...
k_test = bit.PrivateKeyTestnet()
k_test.get_unspents()  # will use the testnet node for API
k_main = bit.PrivateKey()
k_main.get_unspents()  # will use the mainnet node for API
# ...

Remarks

Bitcoin Core is not a blockchain explorer and thus only keeps full track of addresses in its wallet! Therefore to use it as a remote node all addresses generated with Bit and that should be tracked must be added as (watch-only) addresses to Bitcoin Core as follows:

>>> bitcoin-cli importaddress "WATCH_ONLY_ADDRESS"

See importaddress for more details.

It may be useful to add a helper function that can add generated Bit addresses to the Bitcoin Core wallet.

Bitcoin Core RPC behaves particular with regard to coinbase transactions

Coinbase transactions directly received inside a Bitcoin Core wallet address will as expected be returned as a list of Unspent objects when using PrivateKey().get_unused(). However, they will not show up when using PrivateKey().get_transactions(). A fix would be to rewrite the RPCHost method get_transactions to use the RPC method listsinceblock, which returns all transactions associated to the Bitcoin Core wallet but hence also requires Bit to filter them by addresses. As this is an edge case and wasteful for big wallets it has been ignored (for now).
See the discussion here.

TODOs

~~Add test cases~~
~~Add helper function adding addresses as watch-only to the node~~
~~Add documentation~~
~~Check SSL to allow self-signed certs (rpcssl is deprecated, see https://bitcoin.org/en/release/v0.12.0#rpc-ssl-support-dropped)~~

ghost · 2019-05-26T04:23:42Z

Wow, this is awesome! Will need to look closer later.

Thank you!

ofek · 2019-05-27T00:14:12Z

This is so cool!!! I'll try to review sometime this week. Can we add docs?

bjarnemagnussen · 2019-05-27T08:32:04Z

I have added "docs" to the TODO list :)

ofek

This LGTM! Do you think it would be worth it/possible to test this in CI?

bjarnemagnussen · 2019-06-19T12:44:42Z

Yes, I also think we should be able to test it. I will look into this :)

ghost · 2019-06-19T16:08:47Z

Finally looked over the code. I agree that CI testing would be good if possible, but I have to say that this looks great as it is. Very nice work!

ofek · 2019-07-08T04:20:27Z

Can we merge?

bjarnemagnussen · 2019-07-08T06:25:56Z

I have been a bit busy and therefore didn't have the time to add tests yet. But I will look into this in the next two weeks. After that I think we should be ready to merge!

Fixes precision error due to Bitcoin Core node returning the balance in BTC as float instead of in satoshis as integer.

bjarnemagnussen · 2019-07-14T19:42:03Z

I have made two testing classes for the RPCHost and RPCMethod, but I am not sure if this is the most optimal way to do it.

To make the tests I have created two mock classes:

MockRPCMethod, which returns test data when invoked, and
MockRPCHost, which inherits from RPCHost but calls MockRPCMethod instead.

The tests for RPCHost are found inside the class TestRPCHost. It both checks if the initialization is as expected and that the calls to the different functions (which values are returned by MockRPCMethod) are processed as expected.

The tests for RPCMethod are found inside the class TestRPCMethod. It makes use of a module requests_mock, that is used to check that the RPCMethod class does request correctly formatted data from a node. Using requests_mock was for me the only way to easily make those tests possible, but if anyone has a better idea on how to test the requests from RPCMethod let me know!

Due to the import of requests_mock Travis CI is broken right now. If we are going to use this module it should be added to Travis.

ofek · 2019-07-14T19:48:13Z

Great! Sure, add it here

bit/tox.ini

Line 8 in 0b0e553

deps =

ofek · 2019-07-14T20:35:16Z

bit/network/services.py

+            response = self._host._session.post(
+                self._host._url, headers=self._host._headers, data=payload
+            )
+        except requests.exceptions.ConnectionError:


Question: should we catch more exceptions to avoid it printing/logging the url w/ password inside?

Interesting point, but I think logs are generally not regarded as public anyway. I don't see that being a blocker here. Does requests already block passwords, perhaps?

Requests does not, but no need, you're right

I would be open for suggestions to improve this. I am just not sure how.

ghost

Looks pretty good overall, just a few small comments.

ghost · 2019-07-15T20:11:35Z

bit/network/services.py

+    def get_balance(self, address):
+        getcontext().prec = 9
+        b = Decimal(self.getreceivedbyaddress(address, 0))
+        return int(float(str(b * 100000000)))


Don't we already have constants in place for this?

Ah, you are right. They are inside network/rates.py. I will change it to make use of them.

ghost · 2019-07-15T20:13:03Z

bit/network/services.py

+    def broadcast_tx(self, tx_hex):
+        try:
+            _ = self.sendrawtransaction(tx_hex)
+        except BitcoinNodeException:


Would it be worth at least logging the exeption message as a warning? I'm not certain but curious what your thoughts are.

Makes sense. I forgot about it but it is important to find out why a transaction eventually fails to be broadcast. Will add a logging warning!

ghost · 2019-07-15T20:14:06Z

bit/network/services.py

+            response = self._host._session.post(
+                self._host._url, headers=self._host._headers, data=payload
+            )
+        except requests.exceptions.ConnectionError:


Interesting point, but I think logs are generally not regarded as public anyway. I don't see that being a blocker here. Does requests already block passwords, perhaps?

ghost · 2019-07-15T20:17:45Z

tests/network/test_services.py

+            if args[2][0] in (MAIN_ADDRESS_UNUSED, TEST_ADDRESS_UNUSED):
+                return []
+            return [{
+                "txid": "381f1605dd927151fbfac2e88608464414fa5b01bd6298cd1e2d9d991907aa9e",


These are fine, but like with my earlier comment about constants, I think we have samples with a lot of these.

I guess if they're used only once there's no reason to add them as samples.

I agree! However, transaction constants are found inside test_transaction.py, not samples.py. And I think we should probably generally clean up the sample constants. Over time we may have even introduced redundant constants, etc. Maybe this is something that should be done in a future PR, which cleans up the sample constants?

Sure, that's fine.

bjarnemagnussen · 2019-07-16T16:15:05Z

I have added a few more tests, of which especially under TestNetworkAPI the tests test_connect_to_node_main and test_connect_to_node_test are a little bit involved. Basically they test that after calling connect_to_node only the appropriate "constants" inside NetworkAPI are overriden with a RPCHost of correct values. For future compatibility the tests handles connection to multiple RPCHosts.

ghost

Looks great to me. Nice work!

ofek

Great job!!!

bjarnemagnussen · 2019-07-17T08:46:10Z

Thanks! I have just added one little commit. Since we are actually only using the equality method of RPCHost for the tests in TestNetworkAPI, I have moved the method to become a helper function both_rpchosts_equal inside test_services.py.

Removes the equal method from `RPCHost` and adds it as a helper function to `test_services.py`. Also improves a little on the readability of the tests `test_connect_to_node_main` and `test_connect_to_node_test`.

ghost · 2019-07-17T16:12:16Z

Looks great to me! Merge whenever you feel ready.

bjarnemagnussen · 2019-07-21T16:58:08Z

The only last thing I think is missing is a helper function that adds an address to the local node. I will try to add this very soon.

ofek · 2019-08-16T01:11:53Z

Hello again 👋

Can be used to e.g. call `importaddress` on the Bitcoin Core node, etc.

bjarnemagnussen · 2019-08-17T10:16:42Z

Hey again. I hope everyone had a nice summer holiday.

I have actually come to think that the best way to allow adding addresses to the node's wallet is to simply expose direct access the node's RPCs? That is a simple step and I have added documentation of it. Let me know what you think about it this way?

bjarnemagnussen · 2019-08-17T10:30:55Z

Uhhh... Looks like Travis CI is breaking because one of our used addresses MAIN_ADDRESS_USED1 that should have >0 balance belongs to the Bitstamp hacked and was emptied 2 weeks ago (https://www.blockchain.com/btc/address/1L2JsXHPMYuAa9ugvHGLwkdstCPUDemNCf)?

bjarnemagnussen · 2019-08-17T11:21:29Z

We should probably change the MAIN_ADDRESS_USED1 address to a "proof-of-burn" address, e.g. 1CounterpartyXXXXXXXXXXXXXXXUWLpVr. In that regard I am not sure why there are two addresses MAIN_ADDRESS_USED1 and MAIN_ADDRESS_USED2 (and the same for testnet)? However this is something we should fix in a separate PR.

ofek

Excellent! Just left a few fix suggestions for docs formatting

docs/source/guide/remotenode.rst

Co-Authored-By: Ofek Lev <ofekmeister@gmail.com>

bjarnemagnussen · 2019-08-20T21:12:20Z

Just a comment to push notification. I have committed the suggested docs changes!

ofek

Merge away!!!!!

ghost · 2019-08-21T00:55:17Z

Nice work! I'm going to go ahead and merge this.

Adds support for NetworkAPI to use a remote Bitcoin node

588813c

bjarnemagnussen added 3 commits May 27, 2019 13:55

Adds docs for remote node

3a24a17

Allows custom SSL certs when connecting to node

17f8c81

Makes better use of persistent connection for SSL option

8467a3e

ofek approved these changes Jun 15, 2019

View reviewed changes

ghost mentioned this pull request Jul 12, 2019

simple network service for local full-node AustEcon/bitsv#35

Closed

bjarnemagnussen added 2 commits July 14, 2019 21:21

Fixes precision error

6112308

Fixes precision error due to Bitcoin Core node returning the balance in BTC as float instead of in satoshis as integer.

Adds tests for RPCHost and RPCMethod

37a886d

Adds requests_mock dependency to tox

07faa03

ofek requested changes Jul 14, 2019

View reviewed changes

ghost reviewed Jul 15, 2019

View reviewed changes

bjarnemagnussen added 6 commits July 16, 2019 14:48

Refactors units: moved to constants.py

be3110d

Uses units constants to convert btc to satoshis

a6e0d38

Logs warning if broadcasting tx fails not due to connection error

a691a3b

Adds test for better code coverage of broadcasting tx

c21d086

Adds test for unsupported RPCMethod command

5e2e2af

Adds tests for NetworkAPI.connect_to_node

14e537b

ghost approved these changes Jul 16, 2019

View reviewed changes

ofek approved these changes Jul 16, 2019

View reviewed changes

Small refactoring and better readability

bde036a

Removes the equal method from `RPCHost` and adds it as a helper function to `test_services.py`. Also improves a little on the readability of the tests `test_connect_to_node_main` and `test_connect_to_node_test`.

bjarnemagnussen force-pushed the local-node-support branch from d3f0af0 to bde036a Compare July 17, 2019 08:57

ofek approved these changes Jul 17, 2019

View reviewed changes

ghost approved these changes Jul 17, 2019

View reviewed changes

Expose the node's RPCs

ae909dd

Can be used to e.g. call `importaddress` on the Bitcoin Core node, etc.

ofek requested changes Aug 18, 2019

View reviewed changes

Apply suggestions for doc formatting

559ebac

Co-Authored-By: Ofek Lev <ofekmeister@gmail.com>

ofek approved these changes Aug 21, 2019

View reviewed changes

ghost merged commit 991d7f1 into ofek:master Aug 21, 2019

bjarnemagnussen mentioned this pull request Aug 21, 2019

Local node support #62

Closed

bjarnemagnussen deleted the local-node-support branch September 4, 2019 13:32

This pull request was closed.

[WIP] Adds support for NetworkAPI to use a remote Bitcoin node #76

[WIP] Adds support for NetworkAPI to use a remote Bitcoin node #76

Conversation

bjarnemagnussen commented May 25, 2019 • edited Loading

Example code

Remarks

Bitcoin Core RPC behaves particular with regard to coinbase transactions

TODOs

ghost commented May 26, 2019

ofek commented May 27, 2019

bjarnemagnussen commented May 27, 2019

ofek left a comment

Choose a reason for hiding this comment

bjarnemagnussen commented Jun 19, 2019

ghost commented Jun 19, 2019

ofek commented Jul 8, 2019

bjarnemagnussen commented Jul 8, 2019

bjarnemagnussen commented Jul 14, 2019

ofek commented Jul 14, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bjarnemagnussen commented Jul 16, 2019

ghost left a comment

Choose a reason for hiding this comment

ofek left a comment

Choose a reason for hiding this comment

bjarnemagnussen commented Jul 17, 2019

ghost commented Jul 17, 2019

bjarnemagnussen commented Jul 21, 2019

ofek commented Aug 16, 2019

bjarnemagnussen commented Aug 17, 2019

bjarnemagnussen commented Aug 17, 2019

bjarnemagnussen commented Aug 17, 2019

ofek left a comment

Choose a reason for hiding this comment

bjarnemagnussen commented Aug 20, 2019

ofek left a comment

Choose a reason for hiding this comment

ghost commented Aug 21, 2019

[WIP] Adds support for `NetworkAPI` to use a remote Bitcoin node #76

[WIP] Adds support for `NetworkAPI` to use a remote Bitcoin node #76

bjarnemagnussen commented May 25, 2019 •

edited

Loading