Add peer id inlining for small public keys #1230

AgeManning · 2019-08-28T13:49:40Z

This adds peer id inlining to match the behaviour of the go implementation.

Specifically, it addresses the rust side of libp2p/go-libp2p-daemon#137

A few points to note:

This changes the PeerId for secp256k1 and ed25519 keys
The forked multihash crate needed to be updated to include the identity hash. The go-implementation of this allows for up to 2^32 byte identity objects and this is mimicked here. The current rust-implementation relied on fixed hash sizes, where the identity "hash" can have sizes up to this length. There is now split logic in this implementation, between true hashes and the Identity type (which isn't great, feel free modify it, if there is a cleaner solution).

I've tested this with RSA, ed25519 and secp256k1 curves. This now connects and handshakes correctly with the go-daemon. May want to run further tests to ensure nothing upstream gets broken by these changes.

twittner

I am not sure if we want to support "identity" hashes for up to 4 GiB.

core/src/peer_id.rs

misc/multihash/src/lib.rs

AgeManning · 2019-08-28T16:30:42Z

misc/multihash/src/hashes.rs

@@ -60,6 +63,7 @@ impl Hash {
    /// Get the hash length in bytes.
    pub fn size(&self) -> u8 {
        match self {
+            Hash::Identity => 42,


Also note that this is entirely redundant. A go implementation uses this:
https://github.com/multiformats/go-multihash/blob/64ffd94071a5bc58130ea88fd8404408aef993f2/multihash.go#L141

AgeManning · 2019-08-28T16:33:30Z

I am not sure if we want to support "identity" hashes for up to 4 GiB.

I would tend to agree. The only reason I did this, was because the more featured go-implementation of multi-hashes do it. https://github.com/multiformats/go-multihash/blob/master/multihash.go#L258

It shouldnt affect libp2p really as we are only using this for known-sized keys. Happy to reduce this, am indifferent to the size of identity hashes to support.

tomaka · 2019-09-02T13:22:29Z

The PR looks good. The only problem with it would break any existing network using rust-libp2p (which includes Substrate and Polkadot 😅 ).

If you don't mind, I think we should for now accept both hashed and non-hashed keys, but only send out our key hashed. In other words, from_bytes should accept both Identity and SHA2256, but from_public_key should be left untouched for now. Then it a later release, we change from_public_key.

The disadvantage of doing so is that during this transition period, two different PeerIds could represent the same node on the network. This isn't great, but I also don't really see any way this could be attacked.

AgeManning · 2019-09-02T14:56:00Z

This originally came from compatibility issues with other implementations.

If we broadcast a hashed version, the other implementations will reject that multiaddr when trying to connect on that multiaddr, which was the initial problem.

It sounds like I am the only one really with this issue, and our client uses a forked version of rust-libp2p where I've already added these changes. So perhaps if this is not causing any issues for anyone else at the moment, perhaps we leave this PR, until a later release?

However if you want these in now, I'm happy to make the changes you suggest.

tomaka · 2019-09-02T15:30:28Z

So perhaps if this is not causing any issues for anyone else at the moment, perhaps we leave this PR, until a later release?

We definitely want this in master sooner or later.

Also, apparently js-libp2p introduced the inlining between 0.12.2 and 0.12.3, which broke compatibility with rust-libp2p until this PR is in.

AgeManning · 2019-09-02T19:16:39Z

Also, apparently js-libp2p introduced the inlining between 0.12.2 and 0.12.3, which broke compatibility with rust-libp2p until this PR is in.

Yeah, I usually use "the go implementation" as a synonym of "the libp2p-spec" as most of the specs are based off this and anything mismatching, we should consult the go-impl, so not surprising js-libp2p also matches this.

I'll leave this PR in your hands and let you merge/modify next release/when dependent projects are ready.

tomaka · 2019-10-22T13:44:45Z

Small note: let's keep that PR open in order to not forget to finish this change.

tomaka · 2020-01-10T15:10:06Z

I merged master on top of this PR and added a comment.

tomaka · 2020-01-10T15:37:59Z

Tested on Polkadot. Unfortunately doesn't work, and nodes refuse our connections. Will need to figure out what is happening before merging.

tomaka · 2020-01-10T16:29:48Z

The problem comes from here:

rust-libp2p/protocols/secio/src/lib.rs

Line 145 in 481849e

let peer = pubkey.clone().into_peer_id();

At the end of the secio handshake, a node on an older version of libp2p will generate a SHA256 PeerId for the remote and compare it against the expected identity PeerId, which will fail comparison.

Similarly, a node on a newer version will generate an identity PeerId at the end of the secio handshake, which will fail comparison with the expected SHA256 PeerId.

I'm not sure how to fix that. Maybe we can make id(pubkey) compare equal to sha256(hashed_pubkey)?

twittner · 2020-01-14T11:43:09Z

Maybe we can make id(pubkey) compare equal to sha256(hashed_pubkey)?

I think this points into the right direction. The digest of an identity hash equals the original input, so we can always hash the digest with a different algorithm and compare the result with the other one at hand:

impl PartialEq<PeerId> for PeerId {
    fn eq(&self, other: &PeerId) -> bool {
        match (self.multihash.algorithm(), other.multihash.algorithm()) {
            (a, b) if a == b => {
                self.multihash.digest() == other.multihash.digest()
            }
            (multihash::Hash::Identity, b) => {
                let x = multihash::encode(b, self.multihash.digest());
                x.as_ref() == Ok(&other.multihash)
            }
            (a, multihash::Hash::Identity) => {
                let x = multihash::encode(a, other.multihash.digest());
                x.as_ref() == Ok(&self.multihash)
            }
            _ => false
        }
    }
}

Edit: A property test to add:

    #[test]
    fn equals() {
        use multihash::Hash;
        use quickcheck::{Arbitrary, Gen};

        #[derive(Debug, Clone, PartialEq, Eq)]
        struct HashAlgo(Hash);

        impl Arbitrary for HashAlgo {
            fn arbitrary<G: Gen>(g: &mut G) -> Self {
                match g.next_u32() % 16 { // make Hash::Identity more likely
                    0 => HashAlgo(Hash::SHA1),
                    1 => HashAlgo(Hash::SHA2256),
                    2 => HashAlgo(Hash::SHA2512),
                    3 => HashAlgo(Hash::SHA3512),
                    4 => HashAlgo(Hash::SHA3256),
                    5 => HashAlgo(Hash::Keccak256),
                    6 => HashAlgo(Hash::Blake2b512),
                    7 => HashAlgo(Hash::Blake2s256),
                    _ => HashAlgo(Hash::Identity)
                }
            }
        }

        fn property(data: Vec<u8>, algo1: HashAlgo, algo2: HashAlgo) -> bool {
            let a = PeerId { multihash: multihash::encode(algo1.0, &data).unwrap() };
            let b = PeerId { multihash: multihash::encode(algo2.0, &data).unwrap() };

            if algo1 == algo2 || algo1.0 == Hash::Identity || algo2.0 == Hash::Identity {
                a == b
            } else {
                a != b
            }
        }

        quickcheck::quickcheck(property as fn(Vec<u8>, HashAlgo, HashAlgo) -> bool)
    }

tomaka · 2020-01-14T11:49:54Z

Maybe we can make id(pubkey) compare equal to sha256(hashed_pubkey)?

One thing to not forget is that we derive Hash on PeerId, and the hash of peerid(identity(pub)) has to be the same as the one of peerid(sha2(hashed_pub)).

(nothing unfixable, but I thought I'd add a note to not forget)

rklaehn · 2020-01-25T09:00:38Z

Any chance we can get this merged behind a disabled feature flag? It is essential for interop with go-libp2p and presumably also js-libp2p.

I tried to get the chat example to work with go-ipfs, and one problem is that the peer id on the rust side is different than on the go side. Working from this branch, the peer id is identical.

@koivunej thanks for the hint

tomaka · 2020-01-27T10:23:28Z

Any chance we can get this merged behind a disabled feature flag? It is essential for interop with go-libp2p and presumably also js-libp2p.

This PR needs the changes so that PeerIds compare equal across identity and hashed versions.
I wanted to do the changes, but I haven't had the time to yet.

tomaka · 2020-01-29T10:36:13Z

Finished in #1413

Add peer id inlining for small public keys

5cbbe68

twittner reviewed Aug 28, 2019

View reviewed changes

AgeManning commented Aug 28, 2019

View reviewed changes

Apply @twittner suggestions

ceaf7a9

tomaka mentioned this pull request Sep 3, 2019

Add peer id inlining for small public keys #1237

Merged

This was referenced Sep 24, 2019

Support Identity multihash #564

Closed

If peer id length <= 42, use identity hashing #555

Closed

tomaka mentioned this pull request Jan 2, 2020

Example ipfs-kad is broken #1323

Closed

Merge branch 'master' into peerid-inlining

8c7409c

tomaka requested a review from twittner January 10, 2020 15:10

tomaka approved these changes Jan 10, 2020

View reviewed changes

tomaka mentioned this pull request Jan 21, 2020

Support more Multihash-Protocols for PeerId? #409

Closed

tomaka mentioned this pull request Jan 27, 2020

Finish PeerId inlining change #1413

Merged

tomaka closed this Jan 29, 2020

romanb mentioned this pull request Feb 11, 2020

#1436 triggers assertion failure in libp2p-kad #1437

Closed

AgeManning deleted the peerid-inlining branch July 1, 2020 07:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add peer id inlining for small public keys #1230

Add peer id inlining for small public keys #1230

AgeManning commented Aug 28, 2019

twittner left a comment

AgeManning Aug 28, 2019

AgeManning commented Aug 28, 2019 •

edited

Loading

tomaka commented Sep 2, 2019

AgeManning commented Sep 2, 2019

tomaka commented Sep 2, 2019 •

edited

Loading

AgeManning commented Sep 2, 2019 •

edited

Loading

tomaka commented Oct 22, 2019

tomaka commented Jan 10, 2020

tomaka commented Jan 10, 2020

tomaka commented Jan 10, 2020 •

edited

Loading

twittner commented Jan 14, 2020 •

edited

Loading

tomaka commented Jan 14, 2020

rklaehn commented Jan 25, 2020 •

edited

Loading

tomaka commented Jan 27, 2020

tomaka commented Jan 29, 2020

Add peer id inlining for small public keys #1230

Add peer id inlining for small public keys #1230

Conversation

AgeManning commented Aug 28, 2019

twittner left a comment

Choose a reason for hiding this comment

AgeManning Aug 28, 2019

Choose a reason for hiding this comment

AgeManning commented Aug 28, 2019 • edited Loading

tomaka commented Sep 2, 2019

AgeManning commented Sep 2, 2019

tomaka commented Sep 2, 2019 • edited Loading

AgeManning commented Sep 2, 2019 • edited Loading

tomaka commented Oct 22, 2019

tomaka commented Jan 10, 2020

tomaka commented Jan 10, 2020

tomaka commented Jan 10, 2020 • edited Loading

twittner commented Jan 14, 2020 • edited Loading

tomaka commented Jan 14, 2020

rklaehn commented Jan 25, 2020 • edited Loading

tomaka commented Jan 27, 2020

tomaka commented Jan 29, 2020

AgeManning commented Aug 28, 2019 •

edited

Loading

tomaka commented Sep 2, 2019 •

edited

Loading

AgeManning commented Sep 2, 2019 •

edited

Loading

tomaka commented Jan 10, 2020 •

edited

Loading

twittner commented Jan 14, 2020 •

edited

Loading

rklaehn commented Jan 25, 2020 •

edited

Loading