[ETCM-402] Decode messages with the correct protocol version #810

pslaski · 2020-11-24T15:44:15Z

Description

Currently, we are decoding our messages base on information which is the best protocol we support and assumption that the newest protocol includes older protocol versions.

We should use the protocol negotiated with peer.

Proposed Solution

Create a component which can negotiate protocol version and have our capabilities information. Use it to set up correct protocol version in MessageCodec and EtcHelloExchangeState

Important Changes Introduced

introduced protocol negotiator

Testing

I synced with mordor testnet successfully.

mirkoAlic · 2020-11-25T12:03:17Z

src/main/resources/application.conf

  network {
    # Ethereum protocol version
+    # Supported versions:
+    # 63, 64 (experimental version which enables usage of messages with checkpointing information. In the future after ETCM-355, ETCM-356, it will be 66 probably)


(up for discussion) maybe network.protocol-version should be something configured in blockchains.$someNetwork.network.protocol-version, given it defines in some way part of the consensus. I mean i'm worry about someone configuring blockchains.$someNetwork.ecip1097-block-number but not setting up the proper version for network messages.

src/main/scala/io/iohk/ethereum/blockchain/sync/regular/BlockBroadcast.scala

src/main/scala/io/iohk/ethereum/network/rlpx/MessageCodec.scala

src/main/scala/io/iohk/ethereum/network/EtcPeerManagerActor.scala

mirkoAlic · 2020-11-25T13:07:04Z

src/main/scala/io/iohk/ethereum/blockchain/sync/regular/BlockFetcher.scala

-        supervisor ! ProgressProtocol.GotNewBlock(newState.knownTop)
-        fetchBlocks(newState)
-      }
+    case MessageFromPeer(CommonMessages.NewBlock(block, _), peerId) =>


Maybe we could add a comment regarding the needs of adding a new pattern match in case new versions of NewBlock messages were added.

src/main/scala/io/iohk/ethereum/blockchain/sync/regular/BlockBroadcast.scala

src/test/scala/io/iohk/ethereum/network/p2p/MessageDecodersSpec.scala

src/test/scala/io/iohk/ethereum/network/p2p/messages/NewBlockSpec.scala

rtkaczyk · 2020-11-26T15:58:05Z

assumption that the newest protocol includes older protocol versions

What is wrong with that assumption?

rtkaczyk · 2020-11-26T16:09:47Z

src/main/scala/io/iohk/ethereum/network/p2p/messages/ProtocolNegotiator.scala

+  val capabilities =
+    if (protocolVersion == ProtocolVersions.PV64) Capabilities.All else Seq(Eth63Capability)
+
+  // It is relatively simple now, but ETCM-355 and ETCM-356 will complicate it


I was thinking about handling etc capability separately, but probably we will stick with current impl even with PV64 and PV65 added

rtkaczyk · 2020-11-26T16:18:37Z

src/main/scala/io/iohk/ethereum/network/handshaker/EtcHelloExchangeState.scala

+      case Some(ProtocolVersions.PV64) =>
+        EtcNodeStatus64ExchangeState(handshakerConfiguration)
+      case Some(ProtocolVersions.PV63) =>
+        EtcNodeStatus63ExchangeState(handshakerConfiguration)


For consistency: shouldn't it be called EtcNodeStatus60ExchangeState? PV60 is where this message comes from. Likewise, when 64 becomes 66 we will rename EtcNodeStatus64ExchangeState -> EtcNodeStatus66ExchangeState. But when 67, 68... come along we will not be renaming it.

(Yes, I am guilty of calling it 63 in previous versions of the code)

rtkaczyk · 2020-11-26T16:22:44Z

src/main/scala/io/iohk/ethereum/blockchain/sync/regular/BlockFetcher.scala

      supervisor ! ProgressProtocol.GotNewBlock(newState.knownTop)
      fetchBlocks(newState)
+    // Remember to always add match for every new version of NewBlock message
    case MessageFromPeer(CommonMessages.NewBlock(block, _), peerId) =>


#793 (comment)

KonradStaniec · 2020-12-08T10:18:04Z

src/main/scala/io/iohk/ethereum/network/rlpx/MessageCodec.scala

+    if (remotePeerProtocolVersion.isEmpty) {
+      m match {
+        case hello: Hello =>
+          remotePeerProtocolVersion = protocolNegotiator.negotiate(hello.capabilities)


wdyt of protocolNegotiator per connections, and keeping there state about p2pVersion and protocol version in some thread safe manner ? ( just do not like that now we call protocolNegotiator.negotiate(hello.capabilities) in two places with same argument, and if some in the future will look only on EtcHelloExchangeState that this negotiation also happens on layer bellow for decoding purposes)

it something very similar to besu implementation - IMHO it's a good idea, but not for this PR, but for new task :) I will create one

https://jira.iohk.io/browse/ETCM-474

src/test/scala/io/iohk/ethereum/network/p2p/messages/ProtocolNegotiatorSpec.scala

src/main/scala/io/iohk/ethereum/network/p2p/MessageDecoders.scala

pslaski requested review from KonradStaniec, ntallar and rtkaczyk November 24, 2020 15:44

pslaski force-pushed the etcm-402-message-codec-protocol-version-negotiation branch from 7ec3814 to 1d3393b Compare November 24, 2020 15:46

pslaski force-pushed the etcm-280-peer-checkpoint-capability branch 2 times, most recently from b1307b7 to fa3d0d2 Compare November 25, 2020 09:35