Discuss - 0030-nodes_key_as_name about rfcs HOT 20 OPEN

Depending on how we read point 5, it might cause an uneven distribution of nodes: In the implementation we need to make sure that when selecting the nodes furthest apart, we include the lower and upper boundary of the group's interval as "nodes" in the calculation. E.g. if the group deals with [0,15] and has the nodes 11, 14 and 15, we should offer the (middle of the) interval [0,10], not [12,13].

from rfcs.

Well, I guess we could just implement the Xorable trait for crypto::sign::PublicKey in kademlia_routing_table, then NodeId could just be an alias of this rather than double wrapping an array?

from rfcs.

👍 to simplifying the IDs.
However, getting rid of PeerId would mean that these key pairs are also used by Crust, but exposed to Routing?

One potential issue might be that generating key pairs until one ends up in the assigned group could take a long time for a large network, during which the group needs to deny any other node to join (unless we find an alternative to the current join limit).

from rfcs.

I think crust can still use a peer id if it wishes and I think it should) but routing does not need to care about it , apart from it being an identifier for the connection itself, not the node. It should perhaps be a connection ID. I suspect nodes may eventually want multiple connections between peers, not sure but I don't think we should prevent that. We just do not want arbitrary connections from crust that we don't want AFAIK.

Yes on much larger networks it is more difficult, but not wild I do not think, we can create atm on a single thread 10,000 keypairs in 0.57 seconds ( Took 0.534415757 seconds to create 10,000 keys) from a crude test in sodiumoxide I just added..

  #[test]
    fn gen_keypairs() {
        let count = 10_000;
        let now = time::Instant::now();
        (0..count).all(|_| {
            let _ = gen_keypair();
            true
        });
        println!("Took {:?}.{:?} seconds to create {:?} keys",
                 now.elapsed().as_secs(),
                 now.elapsed().subsec_nanos(),
                 count);
    }

I suspect we can gain a lot if we had XorName able to construct from a sign::PublicKey and a box_::PublicKey with the ability to XorName.get_sign_key() and XorName.get_enc_key() Although I feel this should be a type in routing that internally has an XorName and this type is a RoutingId type.

Interested on your input here @Fraser999

from rfcs.

@dirvine: What you're describing sounds a lot like the current PublicId (but not identical). Is that right?

from rfcs.

Yes, but I think only the sig::PublicKey is more than enough, we can transmit other keys if necessary and now we use 256 bit addresses it is fine as just that sign key I think.

from rfcs.

@dirvine Would it be possible to show some further details of the proposed changes in the RFC please? I guess things that aren't clear to me are:

• the name of the proposed new type
• what key type(s) it will hold
• what functions/traits it will implement

from rfcs.

I think it's just a wrapped PublicKey here @Fraser999 On receipt of this ID another node can confirm the id signed the message. It could be the hash but then we also need to transmit the public key so for now I would make the node id just the public key.
If we want to ensure the best distribution nodes can hash this to confirm the network id this node belongs to. i.e. I get your NodeId and it's fraser so I hash this and check if it's close to me. So the network sees you as this hash, but the message only contains the public key.
so
NodeId is a struct that hold only [u8,32] in essence so for us this is just a PublicKey type which at the moment is a crypto::sign::PubicKey

IS that what you are meaning @Fraser999

from rfcs.

Yes - thanks. It's what I originally thought you meant, but #127 (comment) was confusing me.

In that case, I'm not really seeing a need for any new type. The existing XorName exactly fits the bill doesn't it? We could maybe put in a couple of helper function to cast between the types, but they'd be a bit superfluous I think.

from rfcs.

I wonder though, with the XorAble trait we can have different types with the same shared abilities. So perhaps there is a case for a specific type here to represent a NodeName but one that implements XorAble ? I feel the XorName is way overused as a type and causes a bit of confusion. The xorable trait though allows us to name node names, daa hashes etc. as their correct names and not scrub types perhaps.

from rfcs.

for the first node to the network, it still follows the current first process, right?
for the second, follows the current procedure as well? or relocated to range (first, A) (A being the second nodes' original address) ?

from rfcs.

The joining nodes' original ("client") address should certainly not be included in the range definition. And the first node can still choose its name arbitrarily, I think.

So if the first nodes' address first starts with 0, the second node must go in the middle of [first, 111..111], otherwise in the middle of [000..000, first].

from rfcs.

Actually, implementing Xorable for PublicKey won't help, I think, because the type of a data name must be the same as the type of a node name.

So I guess the PublicKey::name function should just return a XorName matching the key?

Edit: We could, of course, drop the XorName wrapper and use [u8; 32] directly, which we'd get out of public keys and data hashes. But unfortunately, PublicKey only seems to return &[u8], not [u8; 32], so that might also be tricky.

from rfcs.

I'm not sure that's definitely the case. If we need to hold these two different types in a single container, I'd agree, but I can't think of such a case. Normally we'd have a collection of nodes (account info/routing table) or a collection of data names.

from rfcs.

So e.g. the routing table would hold node names - but its close_group function will need to be called with both data and node names. So at least the Xorable trait would need to be modified to make them compatible. E.g. cmp_distance would need to accept as the recipient and its arguments either of the two types, and be able to combine them. But it can't just accept any Xorable, because that doesn't specify how many bytes it has.

from rfcs.

Right. And that could get ugly I guess. We'd need to do something like extend Xorable to provide a getter for a slice of the underlying byte array (already slightly problematic for u16, u32, etc.) and use that. Which leads to how to handle XORing different length arrays. Then we could change to

fn cmp_distance<L: Xorable, R: Xorable>(&self, lhs: &L, rhs: &R)

and actually implement it in the trait.

All do-able I suppose, but maybe not as simple or clean as I'd originally thought :(

from rfcs.

According to the discussion in the routing slack channel, here is the summary of the conclusion regarding PublicId, PublicId::name and PublicKey
1, Currently, the PublicId::name is configuratable, and not binding to any keys that public_id contains. With the implementation of the new joining process (routing PR 1115), such configurable name is no longer required any more. So, now the name can be bound to the signing_key (or the encryption_key, or both)
2, the PublicId contains both the encryption_key and the signing_key. Although the encryption_key is not used so far, there is plan it is going to be used soon. So, the exchanging of NodeIdentity message (exchange public_ids of the pair) is still required.
3, Although it is possible to have a NodeId which is public_key only (needs to make sign::PublicKey xorable), given that we still need to exchange encryption_key anyway, there is not much gain on switching from NodeId to be hash of pub_sign_key to pub_sign_key directly.

So my preferred conclusion will be : make PublicId::name non-configuration (i.e. bound to pub_sign_key), and such name to be hash of the pub_sign_key.

@canndrew @Fraser999 your thoughts please

from rfcs.

I guess you were actually seeking @afck's opinion rather than Andrew's? Anyhoo, I agree with points 1 and 2.

For point 3, I agree with removing set_name(), but my preference would be to implement the Xorable trait for PublicKey (along with agreeing and implementing the details to allow us to cmp_distance between any two arbitrary Xorable types as per discussion above).

Then PublicId could have something like

pub fn name(&self) -> &sign::PublicKey {
    &self.public_sign_key
}

and we could get rid of the name member variable. That allows us to avoid the current widespread type-erasure with all names being an XorName and also avoids unnecessary hashing.

Even if we don't go for this, my second choice would be keeping the name member, but just initialise it like name: XorName(public_sign_key.0). The only reasons I can see which justify hashing the key to derive the name are 1. to ensure an even distribution of names across the address space (although having now done some tests, I think the current PublicKey type already achieves that) and 2. to reduce the maintenance overhead if we switch from the current PublicKey type to something which is incompatible with XorName. Both these are fairly weak reasons, but then I don't have particularly strong reasons for not hashing either; really just to simplify the code and avoid burning a few unnecessary cycles.

from rfcs.

I agree with Fraser, but regarding point 3 I'm undecided: I think if it turns out to add a lot of complexity to change Xorable, we should go with the second choice (keep the name member and initialise it accordingly).

from rfcs.

Yes - I wouldn't opt for that either if it introduces a significant amount of further complexity.

from rfcs.