Don't filter out local node ID when returning peer list (continued) #975
Don't filter out local node ID when returning peer list (continued) #975kaykurokawa merged 6 commits intomasterfrom
Conversation
kaykurokawa
changed the title
kaykurokawa
force-pushed
the
ordex-dht-list-fix
branch
2 times, most recently
from
3ca71ab to
182eed0
Compare
kaykurokawa
changed the title
| if self.node_id != peer[6:]: | ||
| host = ".".join([str(ord(d)) for d in peer[:4]]) | ||
| if host == "127.0.0.1" and "from_peer" in result \ | ||
| and result["from_peer"] != "self": |
There was a problem hiding this comment.
This is coming from https://github.com/lbryio/lbry/blob/0b771c16ba058d3919b2bac482f4d725473a72fa/lbrynet/dht/node.py#L369
Is removing the from_peer handling intentional?
kaykurokawa
force-pushed
the
ordex-dht-list-fix
branch
from
182eed0 to
90060e9
Compare
|
After discussion with jack , it looks like his "from_peer" field never gets used except in the if statement we just removed. I added a commit 90060e9 that completely removes the addition of this "from_peer" field. Also rebased to master. |
| blob_hash (str): blob hash to look for | ||
| timeout (int): seconds to timeout after | ||
| filter_self (bool): if True, and if a peer for a blob is itself, filter it | ||
| from the result |
There was a problem hiding this comment.
(assume that I have not dug yet in the downloader code) I am wondering if knowing that self already has the blob could be a useful information to avoid downloading it again. Or is this case handled differently by the downloader (i.e. the downloader checks the blob directory directly before issuing a request)?
There was a problem hiding this comment.
There is a BlobManager class which knows all the blobs it has. In theory , when the daemon is downloading it should not try to download blobs it has announced. But I think in practice, this can happen if for example you announce a blob, and than you delete the blob in blob manager and try to download the same blob again.
There was a problem hiding this comment.
OK, it makes sense. Thanks for clarifying!
kaykurokawa
force-pushed
the
ordex-dht-list-fix
branch
from
90060e9 to
42567c2
Compare
|
Rebased to master, |
If a node is returning a peer list for a given blob hash (being this been requested via CLI or via DHT) and it is part of the resulting peer list, it will filter itself out before returning the list. This makes the results across the DHT inconsistent as different nodes won't include themselves when responding a findValue/findNode query. Remove such filtering so that the local node ID is always included when needed. Signed-off-by: Antonio Quartulli <antonio@mandelbit.com>
…s blobs, instead of specifying it in announceHaveBlob
… from peer list. Use this argument to remove itself from peer list when downloading blobs do not filter self on peer list
kaykurokawa
force-pushed
the
ordex-dht-list-fix
branch
from
42567c2 to
b380f5d
Compare
3 participants
Continuing from #968
Removed the "if" statement in dht.node.getPeersForBlob(), where it did some strange workaround for filtering localhosts from the results.
I added argument "filter_self" to DHTPeerFinder.find_peers_for_blob() where it will filter itself from peer results if True, this option is needed when downloading blobs since you don't want to download blobs from yourself. In other use cases, such as API command peer_list, filter_self is defaulted to False so you will be able to find yourself. Before #968, the filtering of itself was done in the DHT code and would actually affect availability. Now it is done purely outside of the DHT code so it has no impact on availability.
In order for DHTPeerFinder to know whether a peer was itself or not, I needed to add the "peerPort" as member variable that is initialized by the dht.node class. Thus when announcing blobs using dht.node.announceHaveBlob(), port is no longer needed as an argument. I think this is a more logical way of doing it since externalIP is already a member variable of the dht.node class and there is no use case where we need to announce blobs on different ports.