search

libp2p / py-libp2p

The Python implementation of the libp2p networking stack 🐍 [under development]
https://libp2p.io
Other
481 stars 105 forks source link

Update digest hash function to sha256 #197

Open NIC619 opened 5 years ago

NIC619 commented 5 years ago

What's wrong?

https://github.com/libp2p/py-libp2p/blob/1727ba48d9e5cff4fb00d3972aee0a7cd1871eb0/libp2p/peer/id.py#L87-L90 sha1 is used in calculating a DHT ID

  1. XOR ID of a peer https://github.com/libp2p/py-libp2p/blob/1727ba48d9e5cff4fb00d3972aee0a7cd1871eb0/libp2p/peer/id.py#L37-L38
  2. key in DHT https://github.com/libp2p/py-libp2p/blob/1727ba48d9e5cff4fb00d3972aee0a7cd1871eb0/libp2p/kademlia/network.py#L143

How can it be fixed?

Change it from sha1 to sha256 Go: https://github.com/libp2p/go-libp2p-kbucket/blob/3752ea0128fd84b4fef0a66739b8ca95c8a471b6/util.go#L43-L46

ShadowJonathan commented 4 years ago

Doesn't go-libp2p and js-libp2p, and the general libp2p spec, use sha256 for peer IDs by default and by recommendation?

ShadowJonathan commented 4 years ago

Just looked it up, go-libp2p uses sha256:

https://github.com/libp2p/go-libp2p-core/blob/ba9101b58934544e0faa255cf826e2010d8a0973/peer/peer.go#L227

(Edit: I just saw that other link above, but in this link is it's network peerID hashing counterpart used in its identify protocol)

ShadowJonathan commented 4 years ago

I realise this is even more important, as else py-libp2p is not able to join the current established network, which uses sha256

ralexstokes commented 4 years ago

There are two different things going on here -- peers in libp2p have an ID which is generally encoded as a multihash value like you see here: https://github.com/libp2p/py-libp2p/blob/master/libp2p/peer/id.py#L83-L89. So the specific hash function is specified by the creator of the multihash.

This issue refers to the value used as a key into the kademlia DHT which does happen to use SHA256 per the link in the first comment here: https://github.com/libp2p/go-libp2p-kbucket/blob/3752ea0128fd84b4fef0a66739b8ca95c8a471b6/util.go#L43-L46.

ShadowJonathan commented 4 years ago

Hmmm, I'll look at that and see if I can update/refactor that to use a default hashing function (sha256 cuz why not) in a generic function, or maybe even work it away in utils

ralexstokes commented 4 years ago

i replied to the PR w/ my thoughts on code organization for this one function

ShadowJonathan commented 4 years ago

Which in my eyes sounded similar to what i was implying here, i generally wanna take https://github.com/libp2p/py-libp2p/blob/master/libp2p/peer/id.py#L83-L89 (like you mentioned), the specific multihash.Func class, and implement that uniformly