Support eth and consensus subprotocols #1129

libby · 2021-02-10T19:04:13Z

Enable the eth service to support multiple subprtocols, e.g.
"eth" AND "istanbul/100", instead of a single protocol, e.g. "eth" OR "istanbul/99".

When istanbul consensus was originally added, the eth service was
modified to override the eth subprotocol maintained by the eth service
to an quorum specific consensus protocol, e.g. "istanbul/99" meaning all
eth p2p messages would be sent over a consensus specific protocol in
addition to any consensus protocol message. In the case of istanbul,
the "eth" subprotocol would no long be included as a devp2p capability/
subprotocol for the node.

This commit changes that, and enables the eth service to return multiple
subprotocols, e.g. "eth", "istanbul/100".

"istanbul/100" has been added as a new istanbul subprotocol version, which supports
the multple subprotocols design. peers running older versions of istanbul will
continue to override the "eth" service and will communicate over a single subprotocol:

istanbul/99 <-> istanbul/99
istanbul/100, eth <-> istanbul/100, eth

To maintain backwards compatibility, when establishing a connection with a
new peer that does not support multiple protocols, but rather a "legacy"
protocol, e.g. "istanbul/64" "istanbul/99" "clique" (v2.6), the nodes will communicate
over the single "legacy" protocol and not "eth" + consensus subprotocol.

When running two subprotocols, "eth" + "consensus", the eth subprotocol
manages the lifecycle of the peer for the eth service, and the consensus
subprotocol uses this peer for its communication, registering a separate
protocol read writer on it.

maintains backwards compatibility with: istanbul, raft, clique.

Enable the eth service to support multiple subprotocols, e.g. "eth" AND "istanbul/100", instead of a single protocol, e.g. "eth" OR "istanbul/99". When istanbul consensus was originally added, the eth service was modified to override the eth subprotocol maintained by the eth service to a quorum specific consensus protocol, e.g. "istanbul/99" meaning all eth p2p messages would be sent over a consensus specific subprotocol in addition to any consensus protocol message. In the case of istanbul, the "eth" subprotocol would no long be included as a devp2p capability/ subprotocol for the node. This commit changes that, and enables the eth service to return multiple subprotocols, e.g. "eth" and "istanbul/100". "istanbul/100" has been added as a new istanbul subprotocol version, which supports the multple subprotocol design. peers running older versions of istanbul will continue to override the "eth" service and will communicate over a single subprotocol: old node: istanbul/99 <-> istanbul/99 new node: istanbul/100, eth <-> istanbul/100, eth To maintain backwards compatibility, when establishing a connection with a new peer that does not support mutliple protocols, but rather a "legacy" protocol, e.g. "istanbul/64" "istanbul/99" "clique" (v2.6), the nodes will communicate over the single "legacy" protcol and not "eth" + consensus subprotocol. When running two subprotcols, "eth" + "istanbul" (consensus), the "eth" subprotocol manages the lifecycle of the peer for the eth service, and the consensus subprotocol uses this peer for its communication, registering a separate protocol read writer on it. To support this, a channel is added to a p2p peer (EthPeerRegistered), which is used to communicate to the "consensus" / quorum subprotocols that the eth peer is ready and can be retrieved. When running a quorum specific subprotocol on the eth service, the eth peer is started and managed by the "eth" subprotocol. The quorum specific subprotocol uses this peer to send messages. This channel is necessary, as every protocol registerd in a p2p peer running map, the Run method is called inside a goroutine. When iterating through the running map, the order is not guaranteed and inside the goroutines we cannot count on execution time of the methods. Thus, we use the EthPeerRegistered channel to ensure that the eth protocol has register the peer before attempting to look it up. maintains backwards compatiblity with: istanbul, raft, clique.

When registering a peer check that the protocol is "eth" and version >= eth65 and not a legacy quorum protocol to determine if p.announceTransactions() can be called.

libby · 2021-02-10T19:05:01Z

Following scenarios scenarios should be tested for this PR:

A network running all new nodes (istanbul, clique, raft). These nodes should agree on istanbul/100 and eth/64 subprotcols.
A network running new nodes, and the latest stable version 20.10.0

This can be done using qctl and minikube see Consensys/qubernetes#120
build a local docker image from this branch using the steps in qubernetes

> git clone https://github.com/ConsenSysQuorum/quorum.git
> cd quorum
> git checkout support-eth-and-consensus-protos

start minikube

minikube start --memory 6144

copy the file in dev/docker-helpers/ from Consensys/qubernetes#120 to the base quorum directory
Run the script to build the local container, we'll call it quorum-mul-protos

./quorum-build-all.sh quorum-mul-protos

check that the image was built in the minikube docker env

$> eval $(minikube docker-env)
$> docker images
quorum-mul-protos                         latest        51502f91c6f1   About an hour ago   1.29GB
...
k8s.gcr.io/pause                          3.1           da86e6ba6ca1   3 years ago         742kB
gcr.io/k8s-minikube/storage-provisioner   v1.8.1        4689081edb10   3 years ago         80.8MB

At this point there will be a local image in the minikube docker context that we can access when creating our local quorum networks.

Create a Quorum network of new nodes

$> qctl init --consensus=istanbul --qimagefull=quorum-mul-protos --num 3
$> qctl generate network --create
$> qctl deploy network --wait

Create a Quorum network of new nodes and latest stable release

$> qctl init --consensus=istanbul --qimagefull=quorum-mul-protos --num 3

Add 1 or more stable release nodes to the minimal config

$> qctl add node --qversion=20.10.0 quorum-node4

create and generate the network

$> qctl generate network --create
$> qctl deploy network --wait

once the network is deployed

connect to a node

$> qctl connect node1

open the logs in vi

$> vi $QHOME/quorum.log

search for 'Starting Protocol' to see the protocols that each nodes uses to connect.

ricardolyn

is there a plan to add Acceptance Tests for this case?

eth/backend.go

ricardolyn · 2021-02-11T10:25:59Z

eth/quorum_protocol.go

+// The Run method starts the protocol and is called by the p2p server. The quorum consensus subprotocol,
+// leverages the peer created and managed by the "eth" subprotocol.
+// The quorum consensus protocol requires that the "eth" protocol is running as well.
+func (pm *ProtocolManager) makeQuorumConsensusProtocol(ProtoName string, version uint, length uint64) p2p.Protocol {


do we have unit tests for this new code or other tests that are covering this?

nmvalera · 2021-02-10T21:03:03Z

eth/handler.go

+
+	// Quorum notify other subprotocols that the eth peer is ready, and has been added to the peerset.
+	select {
+	case p.EthPeerRegistered <- true:


I am not fan of notifying through a chan bool

A more standard option is to use a chan struct{} and close it to broadcast notification.

In this case, since it is required to notify true or false I would instead use 2 channels

EthPeerReady

EthPeerDisconnected

Changed EthPeerRegistered to buffered chan struct{}{} and added EthPeerDisconnected instead of using chan bool and sending true false.

nmvalera · 2021-02-11T10:04:06Z

eth/quorum_protocol.go

+
+// istanbul/64, istanbul/99, clique/63, clique/64 all override the "eth" subprotocol.
+func isLegacyProtocol(name string, version uint) bool {
+	legacyProtocols := map[string][]uint{"istanbul": {64, 99}, "clique": {63, 64}}


Can we declare legacyProtocols as a global variable?

We can, but it is only used in the isLegacyProtocol method atm.

nmvalera · 2021-02-11T16:55:43Z

eth/backend.go

-		protocolName = quorumProtocol.Name
-		ProtocolVersions = quorumProtocol.Versions
+		// set the quorum specific consensus devp2p subprotocol, eth subprotocol remains set to protocolName as in upstream geth.
+		quorumConsensusProtocolName = quorumProtocol.Name


Is it necessary to use quorumConsensusProtocolName, quorumConsensusProtocolVersions, etc.
global variable?

We are mirroring the way the eth protocolName ProtocolVersions is set for consistency:
eth/backend.go

func (s *Ethereum) Protocols() []p2p.Protocol { protos := make([]p2p.Protocol, len(ProtocolVersions)) for i, vsn := range ProtocolVersions {

func (s *Ethereum) quorumConsensusProtocols() []p2p.Protocol { protos := make([]p2p.Protocol, len(quorumConsensusProtocolVersions)) for i, vsn := range quorumConsensusProtocolVersions {

Ok I get the point.

Still I see a difference

func (s *Ethereum) Protocols() []p2p.Protocol { protos := make([]p2p.Protocol, len(ProtocolVersions)) for i, vsn := range ProtocolVersions {

uses ProtocolVersions global variable which is constant while

func (s *Ethereum) quorumConsensusProtocols() []p2p.Protocol { protos := make([]p2p.Protocol, len(quorumConsensusProtocolVersions)) for i, vsn := range quorumConsensusProtocolVersions {

uses quorumConsensusProtocolVersions global variable which is not constant.

quorumConsensusProtocolVersions is indirectly set by New(...) (*Ethereum, error) which I think is not a good practice and maybe error-prone (e.g it creates coupling between Ethereum structs through a global variable which is not constant).

I think it would be better to have a quorumConsensusProtocolVersions attribute on the Ethereum struct that is set on New(...) then quorumConsensusProtocols() could re-use the attribute or something like this.

What do you think?

Before this change, the ProtocolVersions and ProtocolName vars would be overridden here to set them to the quorum specific subprototol obtained from the consensus engine. In upstream (same geth version), they would not be overridden. This change leaves ProtocolName ProtocolVersions always set to the eth subprotocol.
With this initial dual subprotocol approach, keeping the changes as minimally invasive to the existing code as possible was the approach. In the (near) future, this will be refactored more (consensus may be a service with its own protocol, when we get up to date with the current geth, eth service will be refactored to support more than one subprotocol, e.g. snap, eth, quorumconsensus. To the client, they won't see a difference once they are running the dual subprotocols, but the Quorum code can refactored to support a different subprotocol design.

make EthPeerRegistered a buffered chan struct{} instead of a chan bool, add EthPeerDisconnected chan to listen for peer disconnect.

nmvalera

LGTM

libby added 3 commits February 10, 2021 13:23

notify ethPeerRegistered channel if peer disconnected.

7d4086f

eth: eth65 and multi eth service protocol.

e3a07c3

When registering a peer check that the protocol is "eth" and version >= eth65 and not a legacy quorum protocol to determine if p.announceTransactions() can be called.

libby requested review from jbhurat and ricardolyn February 10, 2021 19:11

ricardolyn reviewed Feb 11, 2021

View reviewed changes

nmvalera self-requested a review February 11, 2021 10:55

nmvalera reviewed Feb 11, 2021

View reviewed changes

libby force-pushed the support-eth-and-consensus-subprotos branch from 8563d93 to 5f6e74a Compare February 11, 2021 23:29

eth,p2p: make EthPeerRegistered chan struct{}.

49fedfc

make EthPeerRegistered a buffered chan struct{} instead of a chan bool, add EthPeerDisconnected chan to listen for peer disconnect.

libby force-pushed the support-eth-and-consensus-subprotos branch from 5f6e74a to 49fedfc Compare February 12, 2021 00:40

nmvalera approved these changes Feb 12, 2021

View reviewed changes

nmvalera merged commit 507c858 into Consensys:master Feb 12, 2021

ricardolyn mentioned this pull request Feb 12, 2021

[Upgrade] Go-Ethereum release v1.9.13 #1123

Merged

15 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support eth and consensus subprotocols #1129

Support eth and consensus subprotocols #1129

libby commented Feb 10, 2021

libby commented Feb 10, 2021

ricardolyn left a comment

ricardolyn Feb 11, 2021

nmvalera Feb 10, 2021

libby Feb 11, 2021 •

edited

nmvalera Feb 11, 2021

libby Feb 11, 2021 •

edited

nmvalera Feb 11, 2021

libby Feb 11, 2021

nmvalera Feb 12, 2021

libby Feb 12, 2021

nmvalera Feb 12, 2021

nmvalera left a comment

Support eth and consensus subprotocols #1129

Support eth and consensus subprotocols #1129

Conversation

libby commented Feb 10, 2021

libby commented Feb 10, 2021

Create a Quorum network of new nodes

Create a Quorum network of new nodes and latest stable release

once the network is deployed

ricardolyn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

libby Feb 11, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

libby Feb 11, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nmvalera left a comment

Choose a reason for hiding this comment

libby Feb 11, 2021 •

edited

libby Feb 11, 2021 •

edited