[ETCM-280] Use capabilities when sending checkpoint related network messages #793

pslaski · 2020-11-13T12:17:55Z

Description

The version of the message to send should be determined by peer capabilities

Proposed Solution

During the handshake, the peer is introducing its capabilities to supported protocols. Base on that mantis is deciding which version of message use.

Important Changes Introduced

new protocol version 64 with new NewBlock and Status messages (can be configured by network.protocol-version)
decouple NewBlock and Status messages from our services

Testing

Enable PV64 and try to sync with PV63 client and PV64 client

ntallar

I like how this is all looking! I still have pending taking a look at the test changes, some comments before that:

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

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

ntallar · 2020-11-13T15:27:00Z

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

@@ -24,10 +26,14 @@ case class EtcHelloExchangeState(handshakerConfiguration: EtcHandshakerConfigura

  override def applyResponseMessage: PartialFunction[Message, HandshakerState[PeerInfo]] = { case hello: Hello =>
    log.debug("Protocol handshake finished with peer ({})", hello)
-    if (hello.capabilities.contains(Capability("eth", Versions.PV63.toByte)))
-      EtcNodeStatusExchangeState(handshakerConfiguration)
+    if (handshakerConfiguration.protocolVersion == Versions.PV64 && hello.capabilities.contains(Eth64Capability))


Something that I'm thinking about this now... this will be all fine and well for a testnet with Mantis only nodes, but when we open it up for any other client to implement checkpointing/treasury and connect to it, this will sound like a hack very difficult for them to do so.

And I don't see any way of working our way around it... if you agree with what I'm saying I'll share this with Dragan and Juli so that they know about it and maybe give higher priority to implementing eth64 and eth65

I agree - current PV64 version should be temporary, only for our testnet usage and ultimately it should be PV65/PV66 (I'm not sure what is last ETC PV)

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

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

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

rtkaczyk · 2020-11-18T08:31:40Z

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

+          chainWeight.totalDifficulty,
+          bestHash,
+          genesisHash,
+          chainWeight.lastCheckpointNumber


This is really minor, but I see you keep insisting on having this as last field in the RLP. I really don't see the point in this. This is not an extension, all the fields could be totally different. It would make much more sense to me to keep relevant fields close together.

rtkaczyk · 2020-11-18T08:34:07Z

src/test/scala/io/iohk/ethereum/network/EtcPeerManagerSpec.scala

+    requestSender.expectMsg(PeerInfoResponse(Some(peer1Info.withChainWeight(ChainWeight.totalDifficultyOnly(newBlock.totalDifficulty)))))
+  }
+
+  it should "update the peer total difficulty when receiving a PV64.NewBlock" in new TestSetup {


total diff -> chain weight ?

rtkaczyk · 2020-11-18T09:15:54Z

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

-      case (PV62 | PV63, pv62.BlockHeaders.code) => payload.toBlockHeaders
-      case (PV62 | PV63, pv62.GetBlockBodies.code) => payload.toGetBlockBodies
-      case (PV62 | PV63, pv62.BlockBodies.code) => payload.toBlockBodies
+      case (PV62 | PV63, Codes.NewBlockHashesCode) => payload.toNewBlockHashes


Shouldn't these messages be handled for PV64 as well? This code is getting a bit out of control so I would suggest to refactor as follows:

def fromBytes(protocolVersion, msgCode, payload) = { protocolVersion match { case PV64 => handlePV64(msgCode, payload) case PV63 => handlePV63(msgCode, payload) ... case PV60 => handlePV60(msgCode, payload) case _ => fail() } } private def handlePV64(msgCode, payload) = msgCode match { case pv64msg1 => decode(payload) case pv64msg2 => decode(payload) case msgObsoletedByPV64 => fail() // not sure if we need this case for any PV** ... case _ => handlePV63(msg, payload) } private def handlePV63(msgCode, payload) = msgCode match { case pv63msg1 => decode(payload) case pv63msg2 => decode(payload) ... case _ => handlePV62(msg, payload) } ... private def handlePV60(msgCode, payload) = msgCode match { case pv60msg1 => decode(payload) case pv60msg2 => decode(payload) ... case _ => fail() }

This introduces PV60 as the version for common messages. Perhaps we should even rename CommonMessages to PV60 as this would give us more consistent naming?

In my initial version, Status and NewBlock were part of PV63, but I decided later to move them to CommonMessages again. The true is PV61 Status is different than other ones, but we have not supported it and just use that common Status everywhere besides new PV. I don't know if we want to clean it up or not? @ntallar ? If yes, we can do this in the different PR, as this one is already big one.

rtkaczyk · 2020-11-19T10:38:53Z

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

@@ -38,45 +35,87 @@ object NetworkMessageDecoder extends MessageDecoder {
 // scalastyle:off
 object EthereumMessageDecoder extends MessageDecoder {

-  override def fromBytes(msgCode: Int, payload: Array[Byte], protocolVersion: Version): Message =
+  private type MessageHandler = PartialFunction[(Int, Array[Byte]), Message]


How is this approach better/cleaner than what I proposed?

It is far more composable - with your approach I can't just do case _ => handlePV63(msg, payload) because it isn't true, we are handling NewBlock/Status differently than PV63, so it ends up with copying all those PV63 and PV62 messages or doing similar partition as I did and instead of orElse use case _ => nextHandler(msg, payload) where nextHandler will have another call with wildcard etc. For me it's just the perfect fit for PartialFunction

I can't just do case _ => handlePV63(msg, payload) because it isn't true

What isn't true?

ok you right, I can do this with simple matching, I forgot that those new messages would be matched firstly and the bad path is not reachable

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

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

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

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

ntallar

Only major comment regarding the block broadcasting compatibility issue

Apart from this comments, LGTM!

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

src/test/scala/io/iohk/ethereum/ObjectGenerators.scala

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

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

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

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

ntallar

Apart from the two comments above, LGTM!

ntallar

LGTM!

 .\\            //.
. \ \          / /.
.\  ,\     /` /,.-
 -.   \  /'/ /  .
 ` -   `-'  \  -
   '.       /.\`
      -    .-
      :`//.'
      .`.'
      .' BP

src/main/resources/application.conf

mirkoAlic · 2020-11-26T14:50:26Z

I reviewed this in the following one(#810), where i wrote some comments...But in general, the code LGTM!

rtkaczyk · 2020-11-26T15:39:41Z

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

-      case (PV62 | PV63, pv62.BlockHeaders.code) => payload.toBlockHeaders
-      case (PV62 | PV63, pv62.GetBlockBodies.code) => payload.toGetBlockBodies
-      case (PV62 | PV63, pv62.BlockBodies.code) => payload.toBlockBodies
+  private def handleCommonMessages(msgCode: Int, payload: Array[Byte]): Message = {


Why do we not want to call them PV60?

I think CommonMessages may be misleading. Since PV64 overrides Status and NewBlock, they are not common anymore

#793 (comment) ;)

I can refactor it in ETCM-402

rtkaczyk · 2020-11-26T15:40:37Z

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

+      case PV61 => handlePV61(msgCode, payload)
+      case PV62 => handlePV62(msgCode, payload)
+      case PV63 => handlePV63(msgCode, payload)
+      case PV64 => handlePV64(msgCode, payload)


Very minor optimization: higher PVs will be matched more often, so they could be matched first

rtkaczyk · 2020-11-26T15:41:04Z

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

-      case (_, NewBlock.code63 | NewBlock.code64) => payload.toNewBlock(msgCode)
-
-      case (PV61, t) => handlePV61(t, payload)
+  override def fromBytes(msgCode: Int, payload: Array[Byte], protocolVersion: Version): Message = {


This looks very tidy now <3

rtkaczyk · 2020-11-26T15:48:23Z

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

+      case Disconnect.code => payload.toDisconnect
+      case Ping.code => payload.toPing
+      case Pong.code => payload.toPong
+      case Hello.code => payload.toHello


…kpoint-related messages

pslaski requested review from rtkaczyk, KonradStaniec and ntallar November 13, 2020 12:17

pslaski force-pushed the etcm-280-peer-checkpoint-capability branch from 3135e5a to 928d2b3 Compare November 13, 2020 15:15

ntallar requested changes Nov 13, 2020

View reviewed changes

pslaski force-pushed the etcm-280-peer-checkpoint-capability branch 3 times, most recently from 0c09056 to 5a36702 Compare November 17, 2020 10:26

rtkaczyk reviewed Nov 18, 2020

View reviewed changes

pslaski force-pushed the etcm-280-peer-checkpoint-capability branch 3 times, most recently from c33def8 to d88ac8f Compare November 18, 2020 19:12

rtkaczyk reviewed Nov 19, 2020

View reviewed changes

ntallar requested changes Nov 19, 2020

View reviewed changes

pslaski force-pushed the etcm-280-peer-checkpoint-capability branch 3 times, most recently from 1591596 to ec3b952 Compare November 20, 2020 15:18

ntallar requested changes Nov 20, 2020

View reviewed changes

pslaski force-pushed the etcm-280-peer-checkpoint-capability branch 2 times, most recently from eb04693 to 80d17db Compare November 24, 2020 14:09

ntallar reviewed Nov 24, 2020

View reviewed changes

src/main/scala/io/iohk/ethereum/network/EtcPeerManagerActor.scala Show resolved Hide resolved

ntallar reviewed Nov 24, 2020

View reviewed changes

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

ntallar approved these changes Nov 25, 2020

View reviewed changes

pslaski force-pushed the etcm-280-peer-checkpoint-capability branch from fa3d0d2 to 298934b Compare November 26, 2020 10:23

ntallar requested a review from mirkoAlic November 26, 2020 13:46

ntallar reviewed Nov 26, 2020

View reviewed changes

src/main/resources/application.conf Show resolved Hide resolved

rtkaczyk reviewed Nov 26, 2020

View reviewed changes

[ETCM-280] use peer capabilities (protocol version) when sending chec…

ee982e8

…kpoint-related messages

pslaski force-pushed the etcm-280-peer-checkpoint-capability branch from 298934b to ee982e8 Compare November 26, 2020 16:12

rtkaczyk mentioned this pull request Nov 26, 2020

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

Merged

ntallar approved these changes Nov 26, 2020

View reviewed changes

ntallar merged commit 302e2dc into develop Nov 26, 2020

ntallar deleted the etcm-280-peer-checkpoint-capability branch November 26, 2020 19:16

ntallar added the BREAKS API Affects services that interacts with APIs label Nov 27, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ETCM-280] Use capabilities when sending checkpoint related network messages #793

[ETCM-280] Use capabilities when sending checkpoint related network messages #793

pslaski commented Nov 13, 2020

ntallar left a comment

ntallar Nov 13, 2020

pslaski Nov 16, 2020

rtkaczyk Nov 18, 2020

rtkaczyk Nov 18, 2020

rtkaczyk Nov 18, 2020

pslaski Nov 18, 2020

rtkaczyk Nov 19, 2020

pslaski Nov 19, 2020

rtkaczyk Nov 19, 2020

pslaski Nov 19, 2020

ntallar left a comment

ntallar left a comment

ntallar left a comment

mirkoAlic commented Nov 26, 2020 •

edited

Loading

rtkaczyk Nov 26, 2020

rtkaczyk Nov 26, 2020

pslaski Nov 26, 2020

pslaski Nov 26, 2020

rtkaczyk Nov 26, 2020 •

edited

Loading

rtkaczyk Nov 26, 2020

rtkaczyk Nov 26, 2020

[ETCM-280] Use capabilities when sending checkpoint related network messages #793

[ETCM-280] Use capabilities when sending checkpoint related network messages #793

Conversation

pslaski commented Nov 13, 2020

Description

Proposed Solution

Important Changes Introduced

Testing

ntallar left a 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

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

ntallar left a comment

Choose a reason for hiding this comment

ntallar left a comment

Choose a reason for hiding this comment

ntallar left a comment

Choose a reason for hiding this comment

mirkoAlic commented Nov 26, 2020 • edited Loading

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

rtkaczyk Nov 26, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mirkoAlic commented Nov 26, 2020 •

edited

Loading

rtkaczyk Nov 26, 2020 •

edited

Loading