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

Extract electrumx module #4427

Closed
wants to merge 27 commits into
base: master
from

Conversation

Projects
None yet
4 participants
@haarts
Contributor

haarts commented Jun 11, 2018

This pull request further clarifies the network module API.

A couple of major things have happened here (non of which impacts functionality):

  • A class Triggers is defined and used as a mixin in the network module. This class deals with GUI notifications.
  • A class ElectrumX is defined and used instead of the Network class. This class holds (almost) all the previously defined ElectrumX specific API calls. The methods therein are just copied over from the Network class.
  • All of the helper methods in the Network class have been prefixed with an underscore as to communicate their private nature.

I would understand if these changes are considered controversial. Read with care.

@kyuupichan

This comment has been minimized.

Collaborator

kyuupichan commented Jun 12, 2018

You seem to have lost some of the changes I recently made

@haarts

This comment has been minimized.

Contributor

haarts commented Jun 12, 2018

I've identified 4 commits of your to which you are referring:

  1. bc83fa8
  2. e0a6b08
  3. b164cc4
  4. 1900e58

Number 1 is addressed in e289880 and 071f782.
Number 2 is rebased properly afaict.
Number 3 I indeed changed slightly and I might have missed something there. I have omitted the server version call in the scripts/block_headers as I believe that that message has already been sent in the initialization of the network class here: https://github.com/haarts/electrum/blob/dbea86f39da57fad4f7677b25635ffa94b445682/lib/network.py#L697
Number 4 I did miss passing True once here: https://github.com/spesmilo/electrum/pull/4427/files#diff-cb3587f39324c05e12dc545e531b2300R120. I will fix that.

I might very well be missing other commits or patches. I appreciate the feedback!

@kyuupichan

This comment has been minimized.

Collaborator

kyuupichan commented Jun 12, 2018

Grepping for get_chunk still has hits. There are none in master.

Otherwise looks good

else:
if (tx_hash not in self.requested_merkle
and tx_hash not in self.merkle_roots):
if tx_height == 0 or tx_height > local_height:

This comment has been minimized.

@SomberNight

SomberNight Jun 12, 2018

Member

you should not let negative tx_heights through; there are some magic numbers in that range; see

electrum/lib/wallet.py

Lines 77 to 79 in 12c5474

TX_HEIGHT_LOCAL = -2
TX_HEIGHT_UNCONF_PARENT = -1
TX_HEIGHT_UNCONFIRMED = 0

This comment has been minimized.

@haarts

haarts Jun 13, 2018

Contributor

Ah, that is surprising. Thanks for pointing that out. Will fix.

@@ -120,7 +120,7 @@ def on_address_history(self, response):
tx_fees = dict(filter(lambda x:x[1] is not None, tx_fees))
# Note if the server hasn't been patched to sort the items properly
if hist != sorted(hist, key=lambda x:x[1]):
self.network.interface.print_error("serving improperly sorted address histories")
self.print_error("error: serving improperly sorted address histories")

This comment has been minimized.

@SomberNight

SomberNight Jun 12, 2018

Member

not that it's criticial but the difference here is that if you called self.network.interface.print_error, it would actually print which server is doing it

This comment has been minimized.

@haarts

haarts Jun 13, 2018

Contributor

Yeah... I know what I did there. The reason I did this was so I could get rid of the last remaining 'interface' reference. I deemed the upside bigger than the downside. I'll leave the code as is if that is OK with you.

@@ -525,7 +525,7 @@ def _add_recent_server(self, server):
def _process_response(self, interface, response, callbacks):
if self.debug:
self.print_error("<--", response)
self.print_error(interface.host, "<--", response)

This comment has been minimized.

@SomberNight

SomberNight Jun 12, 2018

Member

did you test this? I think it's redundant, it happens already.
see

print_error("[%s]" % self.diagnostic_name(), *msg)

@haarts

This comment has been minimized.

Contributor

haarts commented Jun 13, 2018

I left the get_chunk call in the electrumX module with a deprecation comment. But I remove it, that indeed is better.

@haarts

This comment has been minimized.

Contributor

haarts commented Jun 27, 2018

What needs to be done to get this branch merged? (aside from resolving the conflicts)

@ecdsa

This comment has been minimized.

Member

ecdsa commented Jun 27, 2018

@haarts I did not have time to look at it.
note that I also plan to update the network code to use asyncio, so I will look at it at that time.

@haarts

This comment has been minimized.

Contributor

haarts commented Jun 27, 2018

This PR can be seen as a prelude to converting the network code to use asyncio.
As a matter of fact, the code I'm currently work on is yet an other step into that direction. I have kept the network module a thread but only to contain the changes to the network module (otherwise much more intensive refactoring is required). This again is a good step in the direction of making it asyncio compatible.

It would help me a lot if this PR got merged, otherwise my works diverges more and more from master. You can follow my progress on my own branch: https://github.com/haarts/electrum/tree/add-pylibbitcoin

@SomberNight

This comment has been minimized.

Member

SomberNight commented Jun 27, 2018

Given you are subclassing Network to do e-x specific stuff, I am guessing you won't allow libbitcoin/e-x heterogeneous networks. What is your plan for libbitcoin? Will the client connect to multiple libbitcoin instances like with e-x currently, or will it only connect to one which we assume to be trusted?

@haarts

This comment has been minimized.

Contributor

haarts commented Jun 27, 2018

Yes, you are correct. I currently do not allow for hetrogeneous networks. Doing the sub classing as implemented however makes it easy to switch implementations. Either network = ElectrumX() or network = Libbitcoin(). Both implementations have the same interface so the Electrum code can use it as a drop in replacement. That's why spent so much time reducing the API of the Network class. This makes the implementation of the Libbitcoin class doable.

The Libbitcoin implementation supports multiple backends and multiple chains.

@SomberNight

This comment has been minimized.

Member

SomberNight commented Jun 27, 2018

I guess you probably already thought this through but is it easier to modify the client to support both backends (e-x and libbitcoin) than, say having some middleware between the electrum client and libbitcoin translate the electrum protocol calls into libbitcoin rpc (or similar)?

return addr, amount
scripthash = bitcoin.address_to_scripthash(addr)
hash2address[scripthash] = address
return scripthash, amount

This comment has been minimized.

@SomberNight

SomberNight Jun 27, 2018

Member

bitcoin is not imported
instead of hash2address, you want self.hash2address
addr instead of address

This comment has been minimized.

@haarts

haarts Jun 28, 2018

Contributor

I'm a bit disappointed in myself I missed this. Odd. And fixed.

@haarts

This comment has been minimized.

Contributor

haarts commented Jun 28, 2018

As to your middleware question; the thought occurred to me.
And in a way this is what is happening in my next PR, see protocol.py on my development branch.
I feel that which ever road I would have taken this would still have lead to the changes I've done up until this point. All I've done so far is refactor. All with a dual goal; understanding the code base and clearly defining an API. As a pleasant side effect I'm writing tests and gently polish the code somewhat.
Does this make sense?

@@ -34,6 +34,7 @@
sys.exit("install SimpleWebSocketServer")
from . import util
from lib import bitcoin

This comment has been minimized.

@SomberNight

SomberNight Jun 28, 2018

Member

use a relative import; especially as the package structure is different when running from source and when having the lib installed
( #4279 )

This comment has been minimized.

@haarts

haarts Jun 29, 2018

Contributor

I did not know that. Will fix.

@haarts

This comment has been minimized.

Contributor

haarts commented Jul 2, 2018

I've rebased this PR again.

Would it perhaps help if I split this PR up in pieces? It has grown quite a bit.

haarts added some commits May 21, 2018

Move protocol strings from scripts to network
This is again a small step in the right direction. The network module is
going to accumulate more and more of these simple methods. Once
everything is moved into that module, that module is going to be split.

Note that I've left the scripts which use scripts/util.py alone. I
suspect the same functionality can be reached when using just
lib/network.py and that scripts/util.py is obsolete.
Reduce indentation level
This makes the method easier to read.
Add ElectrumX class and reference it
This class is a subclass of Network and doesn't do anything different at the
moment. The idea is to move ElectrumX specifics to this new class.
Move the majority of the ElectrumX code
Much remains to be done but the demarcation is now clear.
To do is moving ElectrumX specific attributes to the electrumx module and
making the network configurable somehow.
Pass list of tuples instead of bare tuple
This was quite a serious bug.
Add some TODO's
It is starting to be hard to keep track of all the loose ends.
Remove deprecated subscribe_to_addresses call
The main motivation of doing so is that the websockets module now no longer
needs to access the h2addr field on the network module. Removing the deprecated
call was a nice bonus.
Remove subscribe_to_address
The backing ElectrumX call is deprecated in favor of blockchain.scripthash.subscribe.
Return early when callback is not None
This was fixed earlier but that fix was lost in an aggresive rebase.
Make address related method delegate to scripthash
Initially I thought that the caller should be responsible for maintaining the
mapping between address and scripthash. That made sense b/c callers accessed
the mapping in the network module directly. And the dictionary mainting the
mapping was accessed by different callers on potentially different threads.
Turns out that maintaining that mapping on the caller site involves quite a bit
of work. Besides using address instead of scripthash makes sense from the users
perspective. The user never thinks in terms of scripthash. The concurrent
access is not a huge deal since we're only adding items and reading singular
values.

haarts added some commits Jun 6, 2018

Add Triggers
The class is extracted from the Network class. It's a Seperation of Concerns
thing. In the end this will make it easier to change the network module.
Note that the lock used here is now specific to the callbacks, it used to be
shared with the lock used in sending API calls. I think this is clearer.
Change visibility of network methods
Doing so clarifies which methods can be used by others and which are strictly
for internal use. I've choosen to use a weaker form of signaling visibility by
using only one leading underscore b/c I do not want to deviate from the norm
set in this project too much.

This diff also introduces the 'server_version' API call.
Revert to direct sending
This way the method call does not wait for a response. It's a little odd that
the codebase is using two different ways of dealing with responses. One is with
callbacks or blocking and the other is a fire and forget mode in which the
responses are later tide to callbacks via message IDs.
Add connection check
I've assumed that having a blockchain (which lives on an interface) implied the
network is connected. This is not always the case, therefor we need this
additional check.
Rename method
The new name reflects better what it does.
Skip negative tx heights too
A transaction height can be negative too, see the wallet modules
TX_HEIGHT_LOCAL and TX_HEIGHT_UNCONF_PARENT constants.
@haarts

This comment has been minimized.

Contributor

haarts commented Jul 11, 2018

I can still split it up if you guys find that easier. I'll be sure to do so in the future at least. :)

@haarts

This comment has been minimized.

Contributor

haarts commented Jul 12, 2018

I'm closing this one in favor of smaller PR's.

@haarts haarts closed this Jul 12, 2018

This was referenced Jul 12, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment