go/neo/neonet: Fix client handshake not to accept server encoding if it is...

go/neo/neonet: Fix client handshake not to accept server encoding if it is different from what client indicated

If the peers encoding is different than our encoding two different
scenarios can happen, because the handshake order is undefined (e.g.
we don't know if our handshake is received before the peer sends its
handshake):

1. Our handshake is received before peer sends its handshake, NEO/py
closes connection if it sees unexpected magic, version, etc.

2. The client already sends a handshake before it proceeds our handshake.
In this case it initally sends us it version, we can extract its encoding,
and only later, once it proceeded our handshake with the bad encoding,
it closes the connection.

Before this patch case (2) wasn't handled correctly by the automatic
encoding detection of 'DialLink'. 'DialLink' simply accepted the
different-than-expected encoding, but once the peer proceeded the nodes
handshake the peer closed the connection and the initially established
and returned link was immediately closed again. Due to this it was good
luck whether connecting with a peer different with an encoding different
from the expected one worked or didn't work (it depended on which handshake
was faster). Now 'DialLink' should reliably find the correct encoding
and return a stable link.

--------

kirr: this is based on the following original patch by Levin: f6b59772

I updated documentation throughout correspondingly and also added
corresponding handshake-specific test in the previous patch.

See kirr/neo!5 and b2da69e2
(go/neo/neonet: Demonstrate problem in handshake with NEO/py) for more
in-depth description of the problem.

go/neo/neonet/newlink.go

@@ -96,23 +96,22 @@ func (e *_EncodingMismatchError) Error() string {
 // handshakeClient implements client-side NEO protocol handshake just after raw
 // connection between 2 nodes was established.
 //
-// Client indicates its version and preferred encoding, but accepts any
-// encoding chosen to use by server.
+// Client indicates its version and encoding.
 //
 // On success raw connection is returned wrapped into NodeLink.
 // On error raw connection is closed.
-func handshakeClient(ctx context.Context, conn net.Conn, version uint32, encPrefer proto.Encoding) (*NodeLink, error) {
-	enc, rxbuf, err := _handshakeClient(ctx, conn, version, encPrefer)
+func handshakeClient(ctx context.Context, conn net.Conn, version uint32, encoding proto.Encoding) (*NodeLink, error) {
+	rxbuf, err := _handshakeClient(ctx, conn, version, encoding)
 	if err != nil {
 		return nil, err
 	}
-	return newNodeLink(conn, enc, _LinkClient, rxbuf), nil
+	return newNodeLink(conn, encoding, _LinkClient, rxbuf), nil
 }
 
 // handshakeServer implements server-side NEO protocol handshake just after raw
 // connection between 2 nodes was established.
 //
-// Server verifies that its version matches Client and accepts client preferred encoding.
+// Server verifies that its version matches Client and accepts client encoding.
 //
 // On success raw connection is returned wrapped into NodeLink.
 // On error raw connection is closed.
@@ -124,7 +123,7 @@ func handshakeServer(ctx context.Context, conn net.Conn, version uint32) (*NodeL
 	return newNodeLink(conn, enc, _LinkServer, rxbuf), nil
 }
 
-func _handshakeClient(ctx context.Context, conn net.Conn, version uint32, encPrefer proto.Encoding) (enc proto.Encoding, rxbuf *xbufReader, err error) {
+func _handshakeClient(ctx context.Context, conn net.Conn, version uint32, encoding proto.Encoding) (rxbuf *xbufReader, err error) {
 	defer func() {
 		if err != nil {
 			err = &_HandshakeError{_LinkClient, conn.LocalAddr(), conn.RemoteAddr(), err}
@@ -136,7 +135,7 @@ func _handshakeClient(ctx context.Context, conn net.Conn, version uint32, encPre
 	var peerEnc proto.Encoding
 	err = xcontext.WithCloseOnErrCancel(ctx, conn, func() error {
 		// tx client hello
-		err := txHello("tx hello", conn, version, encPrefer)
+		err := txHello("tx hello", conn, version, encoding)
 		if err != nil {
 			return err
 		}
@@ -153,15 +152,18 @@ func _handshakeClient(ctx context.Context, conn net.Conn, version uint32, encPre
 			return &_VersionMismatchError{version, peerVer}
 		}
 
+		// verify encoding
+		if peerEnc != encoding {
+			return &_EncodingMismatchError{encoding, peerEnc}
+		}
+
 		return nil
 	})
 	if err != nil {
-		return 0, nil, err
+		return nil, err
 	}
 
-	// use peer encoding (server should return the same, but we are ok if
-	// it asks to switch to different)
-	return peerEnc, rxbuf, nil
+	return rxbuf, nil
 }
 
 func _handshakeServer(ctx context.Context, conn net.Conn, version uint32) (enc proto.Encoding, rxbuf *xbufReader, err error) {
@@ -321,10 +323,23 @@ func DialLink(ctx context.Context, net xnet.Networker, addr string) (link *NodeL
 
 		link, err = handshakeClient(ctx, peerConn, proto.Version, enc)
 
-		// NEO/py closes connection if it sees unexpected magic, version, etc.
-		// -> in such case retry with next encoding trying to autodetect and match server.
+		// If the peers encoding is different than our encoding two different
+		// scenarios can happen, because the handshake order is undefined (e.g.
+		// we don't know if our handshake is received before the peer sends its
+		// handshake):
+		//
+		//	  1. Our handshake is received before peer sends its handshake, NEO/py
+		//       closes connection if it sees unexpected magic, version, etc.
+		//
+		//	  2. The client already sends a handshake before it proceeds our handshake.
+		//       In this case it initally sends us it version, we can extract its encoding,
+		//       and only later, once it proceeded our handshake with the bad encoding,
+		//       closes the connection.
+		//
+		// -> in both cases retry with next encoding trying to autodetect and match server.
 		// -> stop trying on success, or on any other error.
-		if err == nil || !errors.Is(err, io.ErrUnexpectedEOF) {
+		var errEnc *_EncodingMismatchError
+		if err == nil || !(errors.Is(err, io.ErrUnexpectedEOF) || errors.As(err, &errEnc)) {
 			break
 		}
 	}

go/neo/neonet/newlink_test.go

@@ -32,13 +32,10 @@ import (
 	"lab.nexedi.com/kirr/neo/go/neo/proto"
 )
 
-// _xhandshakeClient handshakes as client with encPrefer encoding and verifies that server accepts it.
-func _xhandshakeClient(ctx context.Context, c net.Conn, version uint32, encPrefer proto.Encoding) {
-	enc, _, err := _handshakeClient(ctx, c, version, encPrefer)
+// _xhandshakeClient handshakes as client with specified version and encoding and verifies that server accepts it.
+func _xhandshakeClient(ctx context.Context, c net.Conn, version uint32, encoding proto.Encoding) {
+	_, err := _handshakeClient(ctx, c, version, encoding)
 	exc.Raiseif(err)
-	if enc != encPrefer {
-		exc.Raisef("enc (%c) != encPrefer (%c)", enc, encPrefer)
-	}
 }
 
 // _xhandshakeServer handshakes as server and verifies negotiated encoding to be encOK.
@@ -73,7 +70,7 @@ func _TestHandshake(t *T) {
 	var err1, err2 error
 	wg = xsync.NewWorkGroup(bg)
 	gox(wg, func(ctx context.Context) {
-		_, _, err1 = _handshakeClient(ctx, p1, 1, t.enc)
+		_, err1 = _handshakeClient(ctx, p1, 1, t.enc)
 	})
 	gox(wg, func(ctx context.Context) {
 		_, _, err2 = _handshakeServer(ctx, p2, 2)
@@ -121,7 +118,7 @@ func _TestHandshake(t *T) {
 	}
 	wg = xsync.NewWorkGroup(bg)
 	gox(wg, func(ctx context.Context) {
-		_, _, err = _handshakeClient(ctx, p1, 1, t.enc)
+		_, err = _handshakeClient(ctx, p1, 1, t.enc)
 	})
 	gox(wg, func(ctx context.Context) {
 		wg := xsync.NewWorkGroup(ctx)
@@ -159,7 +156,7 @@ func _TestHandshake(t *T) {
 	p1, p2 = net.Pipe()
 	wg = xsync.NewWorkGroup(bg)
 	gox(wg, func(ctx context.Context) {
-		_, _, err = _handshakeClient(ctx, p1, 1, t.enc)
+		_, err = _handshakeClient(ctx, p1, 1, t.enc)
 	})
 	gox(wg, func(_ context.Context) {
 		xclose(p2)
@@ -194,7 +191,7 @@ func _TestHandshake(t *T) {
 	ctx, cancel := context.WithCancel(bg)
 	wg = xsync.NewWorkGroup(ctx)
 	gox(wg, func(ctx context.Context) {
-		_, _, err = _handshakeClient(ctx, p1, 1, t.enc)
+		_, err = _handshakeClient(ctx, p1, 1, t.enc)
 	})
 	tdelay()
 	cancel()