Discv5 factorize

[cskiraly]

This set of patches tries to break up protocols.nim and encoding.nim into smaller chunks of code,
reducing dependencies and simplifying modifications to the discv5 code.

The main part is to separate protocol.nim into two layers:

• a lower-half handling authentication, encryption, and key exchange. This is now called Transport.
• an upper-half handling DHT messages and request/response semantics. This is still called Protocol.

Another patch separates encoding related to the lower-half from encoding related to the upper-half.

Other patches try to improve semantics of various send messages, and remove some duplicated code.

The last patch removes some dependencies, but also an export from enr.nim. I can imagine this could create
issues, so it can be removed from the set.

[kdeme]

This set of patches tries to break up protocols.nim and encoding.nim into smaller chunks of code, reducing dependencies and simplifying modifications to the discv5 code.

The main part is to separate protocol.nim into two layers:

I'm fine with doing something like this. I'd like to hear a bit more about the context and your use case. What are you trying to achieve?

I've thought in the past about making parts of discv5 generic. But I was then specifically thinking about doing it at a layer which skips the whole wire protocol of the discv5 spec. So, generic nodes queries / lookups. (And on the other side of the spectrum, perhaps the possibility of switching also the actual UDP transport). This was in the context of reusing most of the code, except the wire protocol, for discv4. However, never got to that considering I was unsure if we would still need discv4 and thus if it was worthy the effort. This resurfaced however in the context of our Fluffy project.

With your suggestion in this PR, we split of the messages and their encoding (rlp), from the packet encoding. That sounds good, but it would only allow you to swap in/out either the packet encoding or the protocol messages. So I'm assuming you are planning to reuse only one of these 2 parts in the dagger project?

I think most of the other suggestions are fine too, but I'll do a more thorough review later on.
I would probably drop the message_encoding just in the messages file. There is already rlp encoding going on there anyhow, no need for a new file. Unless you want to move all rlp encoding out there too, and only keep the message definitions, in order to abstract that part also.

[cskiraly]

This set of patches tries to break up protocols.nim and encoding.nim into smaller chunks of code, reducing dependencies and simplifying modifications to the discv5 code.
The main part is to separate protocol.nim into two layers:

I'm fine with doing something like this. I'd like to hear a bit more about the context and your use case. What are you trying to achieve?

I've thought in the past about making parts of discv5 generic. But I was then specifically thinking about doing it at a layer which skips the whole wire protocol of the discv5 spec. So, generic nodes queries / lookups. (And on the other side of the spectrum, perhaps the possibility of switching also the actual UDP transport). This was in the context of reusing most of the code, except the wire protocol, for discv4. However, never got to that considering I was unsure if we would still need discv4 and thus if it was worthy the effort. This resurfaced however in the context of our Fluffy project.

With your suggestion in this PR, we split of the messages and their encoding (rlp), from the packet encoding. That sounds good, but it would only allow you to swap in/out either the packet encoding or the protocol messages. So I'm assuming you are planning to reuse only one of these 2 parts in the dagger project?

I think most of the other suggestions are fine too, but I'll do a more thorough review later on. I would probably drop the message_encoding just in the messages file. There is already rlp encoding going on there anyhow, no need for a new file. Unless you want to move all rlp encoding out there too, and only keep the message definitions, in order to abstract that part also.

Indeed, I see I’ve left some of the RPL part. I’m adding a patch to move that out as well. I think it would make sense.

Now, the broader context: The main thing we need is a fast DHT, that implements the primitives of the libp2p-dht spec, and integrates well with the libp2p world, implemented in Nim.

Ideally, It should be useful both for libp2p-dht (which relies on the underlying stream transport), and for a dht that skips the overhead coming with those streams and goes UDP.

As you can see your code ticks many of these boxes. It is fast, UDP based, with good datagram crypto, it is in Nim, and it already went through several iterations. There are however some things that would need changing, and there are some conflicting requirements:
1, we need some additional messaging (Add/GetProviders, Put/GetValue).
2, message encoding and message IDs are dictated by specs, and these do collide.
3, RLP encoding is not ideal for me, although if it is in another layer, it is not a big issue
4, a mix of 2 worlds of crypto primitives, one based on ENR, and another based on libp2p signed PeerRecord sounds and feels problematic
5, finally, libp2p-dht assumes a dht running on top of stream oriented libp2p transports, which complicates things

Now one by one:

1. Additional messages: These could be even done on top of talk, if considering functionality, but not considering encoding constraints. Anyway, focusing on functionality, these are all defined as starting with a lookup and then going direct. Talk is a bit restrictive in its single message Request/Response model not allowing multiple response datagrams like in findNode, but apart from that, it is almost like on-top-of-talk.

2. specs: this clearly requires separating encoding and the handling of message IDs. Also swapping out layers below, changing functionality, etc. It also means additional messages can’t be on top of talk, but rather next to it.


3. RLP: This is why I try to separate all these binary encoding parts. As you say, your RPL encoding is there is several layers, and I've tried to separate these. It is there in transport, it is there in the message encoding on top, and it is there in the encoding of ENRs. Ideally one could swap any of these independent of each other.


4. Crypto and peer records: this is complicated, with distinct used in both worlds and with slightly different primitives. However, if I would like to use libp2p peer records and your AES-GCM, I need some changes here at least in my version.


5. There are many aspects here, for example differences in the messaging and operation of lookups. The dicv5 distance based lookup is not what libp2p-dht specifies. That one is closer to discv4 in its messages, but different in its iterations due to stream oriented transports. 

I hope this gives you the context. Code-wise I think it would be good to do the factorisation, and then we can see how it allows us to use common generic parts. What do you think?

[kdeme]

I hope this gives you the context.

It certainly does, thanks!

Code-wise I think it would be good to do the factorisation, and then we can see how it allows us to use common generic parts. What do you think?

Moving the messages encoding is definitely fine. Moving what you call the Transport makes mostly sense in the case you would have custom Clients, which would be your use case. Not that needed/interesting for purely discv5, but its fine separating it out, they can be considered separate layers within discv5 I guess.
I'm somewhat more interested in separating out the wire protocol, but this could be a good first step in that direction.
Not so found on the naming used currently though.

It would be good to know how far this gets you already, some additional remarks/info on your points:

1. Additional messages: These could be even done on top of talk, if considering functionality, but not considering encoding constraints. Anyway, focusing on functionality, these are all defined as starting with a lookup and then going direct. Talk is a bit restrictive in its single message Request/Response model not allowing multiple response datagrams like in findNode, but apart from that, it is almost like on-top-of-talk.

Yes, additional messages could be done over talk. And you could choose your encoding, but the outer layer needs to be rlp encoded byte string. And as you mentioned, there are some constraints such as the multiple response problem.
We actually use this system in the Portal network (fluffy client in nimbus-eth1, WIP) to define an additional protocol on top. And the lack of multiple responses is a problem there. However, in Portal network, there are also several DHTs on top which use each this protocol over talk.
For a single DHT project, it is rather ugly to have messages on two different layers. So I'd understand you'd want to get your own custom messages defined at the base layer with the encoding fit for the project.

2. specs: this clearly requires separating encoding and the handling of message IDs.  Also swapping out layers below, changing functionality, etc. It also means additional messages can’t be on top of talk, but rather next to it.

Well, they could I guess, but it would be ugly, see above.

3. RLP: This is why I try to separate all these binary encoding parts. As you say, your RPL encoding is there is several layers, and I've tried to separate these. It is there in transport, it is there in the message encoding on top, and it is there in the encoding of ENRs. Ideally one could swap any of these independent of each other.

The packet encoding doesn't use rlp, only the messages and the ENR. For the messages it seems solved with this PR if you make a custom messages definition / encoding?
ENR would not be solved yet. The ENR is used in the messages, and thus could be replaced if you make that custom. However it can also be provided during the handshake mechanism, which would require additional changes. (aside from the fact that the ENR is stored in Node, which is what is stored in the routing table).

4. Crypto and peer records: this is complicated, with distinct used in both worlds and with slightly different primitives. However, if I would like to use libp2p peer records and your AES-GCM, I need some changes here at least in my version.

I'd have to read up what "libp2p signed PeerRecord" are, but assuming they are similar to ENRs in the sense that give same security guarantees?

5. There are many aspects here, for example differences in the messaging and operation of lookups. The dicv5 distance based lookup is not what libp2p-dht specifies. That one is closer to discv4 in its messages, but different in its iterations due to stream oriented transports. 

Well, not sure what can be done here. Depends if the goal is to have a DHT that can be used efficiently to lookup, or if it is to follow purely libp2p-dht specs. Discv5 has indeed altered the FindNodes message slightly to be distance based, compared to just passing the target like in discv4 or just original Kademlia. IIRC this was adjusted so that a node would know when another node is returning not really its closest neighbours (whether due to bug or malicious).
In the case where you would have custom messages, you could also just adjust that again.

[cskiraly]

I hope this gives you the context.

It certainly does, thanks!

Sorry for coming back to this only now.

Code-wise I think it would be good to do the factorisation, and then we can see how it allows us to use common generic parts. What do you think?

Moving the messages encoding is definitely fine. Moving what you call the Transport makes mostly sense in the case you would have custom Clients, which would be your use case. Not that needed/interesting for purely discv5, but its fine separating it out, they can be considered separate layers within discv5 I guess. I'm somewhat more interested in separating out the wire protocol, but this could be a good first step in that direction. Not so found on the naming used currently though.

Naming was just a first shot. It is essentially a "SecureDatagramTransport" or "AuthenticatedEncryptedDatagramTransport", I guess.
What do you mean by separating out the wire protocol here?

It would be good to know how far this gets you already, some additional remarks/info on your points:

1. Additional messages: These could be even done on top of talk, if considering functionality, but not considering encoding constraints. Anyway, focusing on functionality, these are all defined as starting with a lookup and then going direct. Talk is a bit restrictive in its single message Request/Response model not allowing multiple response datagrams like in findNode, but apart from that, it is almost like on-top-of-talk.

Yes, additional messages could be done over talk. And you could choose your encoding, but the outer layer needs to be rlp encoded byte string. And as you mentioned, there are some constraints such as the multiple response problem. We actually use this system in the Portal network (fluffy client in nimbus-eth1, WIP) to define an additional protocol on top. And the lack of multiple responses is a problem there. However, in Portal network, there are also several DHTs on top which use each this protocol over talk. For a single DHT project, it is rather ugly to have messages on two different layers. So I'd understand you'd want to get your own custom messages defined at the base layer with the encoding fit for the project.

2. specs: this clearly requires separating encoding and the handling of message IDs.  Also swapping out layers below, changing functionality, etc. It also means additional messages can’t be on top of talk, but rather next to it.

Well, they could I guess, but it would be ugly, see above.

3. RLP: This is why I try to separate all these binary encoding parts. As you say, your RPL encoding is there is several layers, and I've tried to separate these. It is there in transport, it is there in the message encoding on top, and it is there in the encoding of ENRs. Ideally one could swap any of these independent of each other.

The packet encoding doesn't use rlp, only the messages and the ENR. For the messages it seems solved with this PR if you make a custom messages definition / encoding? ENR would not be solved yet. The ENR is used in the messages, and thus could be replaced if you make that custom. However it can also be provided during the handshake mechanism, which would require additional changes. (aside from the fact that the ENR is stored in Node, which is what is stored in the routing table).

Right, I think I was confused by still leaving some RLP code in decodeHandshakePacket.

For the Node, it really goes hand-in-hand with the ENR I think. I see it mostly as an in-memory representation of the ENR.

4. Crypto and peer records: this is complicated, with distinct used in both worlds and with slightly different primitives. However, if I would like to use libp2p peer records and your AES-GCM, I need some changes here at least in my version.

I'd have to read up what "libp2p signed PeerRecord" are, but assuming they are similar to ENRs in the sense that give same security guarantees?

They are similar in their base functionality of having the underlay address(es) info and the public key, all signed, although without ENR's generic extensible key/value pairs.

5. There are many aspects here, for example differences in the messaging and operation of lookups. The dicv5 distance based lookup is not what libp2p-dht specifies. That one is closer to discv4 in its messages, but different in its iterations due to stream oriented transports. 

Well, not sure what can be done here. Depends if the goal is to have a DHT that can be used efficiently to lookup, or if it is to follow purely libp2p-dht specs. Discv5 has indeed altered the FindNodes message slightly to be distance based, compared to just passing the target like in discv4 or just original Kademlia. IIRC this was adjusted so that a node would know when another node is returning not really its closest neighbours (whether due to bug or malicious). In the case where you would have custom messages, you could also just adjust that again.

Just curious, do you have a link to an evaluation of the cost of this in lookup speed?

[arnetheduck]

@cskiraly what is the state of this PR? In general, we have a lot of copies of the discovery and kademlia in particular spread out over the projects - it might be a good time to review these and see if they can be unified perhaps

[kdeme]

I might pick up some of this PR while I look into reusing more discv5 code in discv4

[cskiraly]

@kdeme, I'm now back at this codebase, so I will check how can we rebase this to the current master