Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

added session to udp #788

Merged
merged 5 commits into from Apr 10, 2019
Merged

added session to udp #788

merged 5 commits into from Apr 10, 2019

Conversation

y0sher
Copy link
Contributor

@y0sher y0sher commented Apr 4, 2019

still playing with automation testing.. hence the draft

}

// GetIfExist gets a session from the cache based on address
func (s *sessionCache) GetIfExist(addr string) (NetworkSession, error) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why addr is if type string and not net.Addr?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comment is also relevant for the choice to use string as the type of the key in the sessionCache struct

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

net.Addr is an interface, UDPAddr is passed by pointer hence comparison wouldn't work using the bare structs. UDPAddr is composed of byte array for IP and integer for the port. didn't seem like a big difference for me and was less prone to pointer-interface errors.


// GetOrCreate get's a session if it exists, or try to init a new one based on the address and key given.
// *NOTE*: use of dialFunc might actually trigger message sending.
func (s *sessionCache) GetOrCreate(remote string, pubKey p2pcrypto.PublicKey) (NetworkSession, error) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remote should be of type net.Addr

s.expire()
}

session, err := s.dialFunc(remote, pubKey)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

dialFunc might take a long time, you need to avoid from holding the mutex locked while this method is called

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is not entirely true, dialFunc would create a session (should be a cheap, short cryptographic action) and then send a UDP message which again should be cheap and short.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sending a message is not a short op, compared to memory/cpu ops

// simple cache for storing sessions
const maxSessions = 1024

type storedSession struct {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How is this struct different than the ConnectionPool?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In fact they are quite similar, connectionPool is more complicated because it has to synchronize long dials. also connection pool sessions are cached by the PublicKey received in session key exchange. for UDP we can't do this unless we attach the pubkey to each message.

return session, nil
}

func (s *sessionCache) handleIncoming(from string, ns NetworkSession) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

replace string with net.Addr

p2p/net/udp.go Outdated
}

session := createSession(n.local.PrivateKey(), remote)
handshakeMessage, err := generateHandshakeMessage(session, n.config.NetworkID, 1010, n.local.PublicKey())
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

1010? Isn't it defined in the configuration?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, is it the same handshake message that will be sent over a tcp session?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you're right about the configuration and port. I'll fix that. in fact, the port is not used in the message right now since we exchange it in the "Ping" message.
same handshake message for now indeed. when we'll want to optimize discovery we can tailor the handshake message to also send discovery related stuff. but the key-exchange is the same.

p2p/net/udp.go Outdated
if err != nil {
return nil, err
}
_, err = n.conn.WriteToUDP(handshakeMessage, raddr)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need to wait for the handshake response? Can the local node start send data after it sent the handshake request, but before it received the response?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the answer is no, where do you store the fact that the session is waiting for an HS response

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Currently, the session we're using doesn't wait for a response. the initiator knows both the keys and creates a shared secret with it. when the receiver receives the message with the initiator public key, he can create a shared secret with it and his own key.

p2p/net/udp.go Outdated
return session, nil
}

func (n *UDPNet) handleSession(message []byte) (NetworkSession, error) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

handleSession doesn't say much on what the purpose of this method and when it should be called. Rename it or add documentation

@y0sher y0sher marked this pull request as ready for review April 9, 2019 10:47
@y0sher y0sher force-pushed the p2p_udp_session branch 4 times, most recently from 33302c3 to 496c37c Compare April 10, 2019 12:57
if payload := msg.GetPayload(); payload != nil {
data = service.DataBytes{Payload: payload}
} else if wrap := msg.GetMsg(); wrap != nil {
fmt.Println("############ WTF ", msg.Payload)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove

}

protocol := msg.Metadata.NextProtocol
fmt.Printf("############# CALCING %v proto with %v ############\r\n", protocol, data.Bytes())
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove

)

// simple cache for storing sessions
const maxSessions = 1024
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This value has a major implications on the behavior of the node, it should be in the configuration

p2p/net/udp.go Outdated
if err != nil {
return err
}

sealed := ns.SealMessage(data)
final := p2pcrypto.PrependPubkey(sealed, n.local.PublicKey()) // todo: prepend verison+networkid
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove todo

p2p/udp.go Outdated
}

switch x := payload.(type) {
case service.DataBytes:
protomessage.Data = &pb.ProtocolMessage_Payload{Payload: x.Bytes()}
protomessage.Payload = &pb.Payload{Data: &pb.Payload_Payload{x.Bytes()}}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

use createPayload

…true for NetworkID and ClientVersion.

as we can't track when UDP clients update version or change keys. we need every message to include this basic info. the public key is used to identify or create a new shared secret for secured&encrypted communications.
@y0sher y0sher merged commit 95d054f into develop Apr 10, 2019
@y0sher y0sher deleted the p2p_udp_session branch June 25, 2019 07:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants