[ETCM-76] network messages for checkpointing by rtkaczyk · Pull Request #729 · input-output-hk/mantis

rtkaczyk · 2020-10-09T13:01:53Z

Description

Network messages including latest checkpoint number for best peer selection. Depends on #728

pslaski

just minor things

pslaski · 2020-10-22T15:22:00Z

+//    log.warn(
+//      s"""
+//        |
+//        |  KAPIBARA


pslaski · 2020-10-22T15:26:52Z

+                protocolVersion,
+                networkId,
+                totalDifficulty,
+                latestCheckpointNumber,


would it be better to have latestCheckpointNumber as the last field on list, as it's something which might not exist?

Not sure, does it matter?

not now, but IMHO as encoding to the RLP means encode to the list of things it should be easier to decode it having optional fields on the end than having them somewhere in the middle

Well, it's not a case of optionality here. These are different messages with different codes, ie. once the code is determined a completely different structure may follow.
It's not a big deal the order doesn't really matter but these fields are closely related, in fact, after ETCM-263 they will correspond to a single type - ChainWeight, perhaps then it should even be encoded as a nested RLPList? 🤔

pslaski · 2020-10-22T15:32:52Z

    assert(obtainEncoded2 == expectedEncoded2)
  }

+  ignore("Tests for NewBlock64?? - or are the ones in MessagesSerializationSpec sufficient") {}


IMHO modifying newBlockGen to generate also code64 messages should be enough

ntallar

I have other things to analyse but I'll do that tomorrow and add further comments!

ntallar · 2020-10-23T12:27:11Z

I don't like much that whether we send a 63 or 64 version message depends on our block number... on eth when they added eth64 capability (here for example) they made several changes (as far as I understood at least):

announce the new capability on handshake
have the messages that a node sends to it's peers depend on the capability that they have, instead of our block number. If they announce etc64 then we send Status64 and if not Status63

WDYT? The main drawback I see of the current approach is that you'd never be leaving away the old messages, with what I'm proposing eventually a good chunck of the network will have moved to etc64 and you can deprecate eth63 and even mark those peers as bad ones

rtkaczyk · 2020-10-23T12:56:43Z

announce the new capability on handshake

I like the idea, but it sounds like a big refactor. For one, where we currently construct the message we don't even who the recipient is. Separate task?

Also do we need this separate notion of capability? Couldn't we just take advantage of protocolVersion?

ntallar · 2020-10-23T17:16:06Z

Couldn't we just take advantage of protocolVersion?

Tbh I'm not sure what's the purpose of that protocolVersion... but from what I have seen, ETH/ETC updates of this sort are introduced as new capabilities, so we probably have no escape from that

I like the idea, but it sounds like a big refactor. For one, where we currently construct the message we don't even who the recipient is. Separate task?

Why would it be that big? For now I'm seeing the expected changes to be:

Modify our handshake so that the StatusExchangeState receives the capabilities of the peer in some way
Modify our peers information to know if a peer supports etc64 capability or not

What would the benefit be of having this PR merged as is?

rtkaczyk · 2020-10-23T18:46:28Z

Tbh I'm not sure what's the purpose of that protocolVersion... but from what I have seen, ETH/ETC updates of this sort are introduced as new capabilities, so we probably have no escape from that

Do you know of an example in ETC?

Why would it be that big? For now I'm seeing the expected changes to be:

Well, at least medium. We'll also need to change how blocks are broadcast. Currently PeerInfo is not available when constructing the message.

What would the benefit be of having this PR merged as is?

It will unblock ETCM-263

ntallar · 2020-10-23T19:38:23Z

Do you know of an example in ETC?

There were no changes like this to ETC post-ETH fork, there are some examples before that, as eth62 and so forth (but I haven't taken a look at them)

Currently PeerInfo is not available when constructing the message.

Couldn't we start building the message on BlockBroadcast? There we have the peer information (and I actually prefer it being built there, to decouple this network message from the syncing)

It will unblock ETCM-263

But I'm not sure about this 🤔 I don't see this PR as close to the ready version as it was the case with the syncing one, and you are doing both so there won't be much parallelization if merged... but yeah, if you think that there will be a lot of overlapping between this and that then we can leave that refactoring for a future task

ntallar

Apart from minor comments, LGTM!

ntallar · 2020-10-26T19:30:27Z

But I'm not sure about this thinking I don't see this PR as close to the ready version as it was the case with the syncing one, and you are doing both so there won't be much parallelization if merged... but yeah, if you think that there will be a lot of overlapping between this and that then we can leave that refactoring for a future task

@pslaski regarding ☝️ we discussed it with @rtkaczyk and decided to merge this PR as is and complete the protocol changes implementation both with: https://jira.iohk.io/browse/ETCM-280 and https://jira.iohk.io/browse/ETCM-263

ntallar

LGTM!

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

rtkaczyk force-pushed the etcm-77-checkpoint-syncing branch 4 times, most recently from fbd1980 to b59b3af Compare October 15, 2020 16:10

rtkaczyk force-pushed the etcm-77-checkpoint-syncing branch 4 times, most recently from 70672f2 to f0416e0 Compare October 20, 2020 13:25

Base automatically changed from etcm-77-checkpoint-syncing to develop October 21, 2020 18:16

rtkaczyk force-pushed the etcm-76-checkpoint-network-messages branch from 898f733 to fb78ba5 Compare October 21, 2020 19:35

rtkaczyk marked this pull request as ready for review October 21, 2020 19:37

rtkaczyk requested review from ntallar and pslaski October 21, 2020 19:40

rtkaczyk force-pushed the etcm-76-checkpoint-network-messages branch from fb78ba5 to a2ad1a7 Compare October 22, 2020 08:31

pslaski suggested changes Oct 22, 2020

View reviewed changes

ntallar suggested changes Oct 22, 2020

View reviewed changes

Comment thread src/main/scala/io/iohk/ethereum/blockchain/sync/regular/BlockImporter.scala

ntallar added the BREAKS PROTOCOL Affects Node2Node interactions in a way that makes them not interoperate label Oct 23, 2020

rtkaczyk force-pushed the etcm-76-checkpoint-network-messages branch from a2ad1a7 to 34a5958 Compare October 23, 2020 18:55

rtkaczyk force-pushed the etcm-76-checkpoint-network-messages branch 2 times, most recently from cde4e85 to 94985ec Compare October 26, 2020 15:18

ntallar suggested changes Oct 26, 2020

View reviewed changes

rtkaczyk force-pushed the etcm-76-checkpoint-network-messages branch from 94985ec to 024513c Compare October 27, 2020 13:11

ntallar approved these changes Oct 27, 2020

View reviewed changes

pslaski approved these changes Oct 28, 2020

View reviewed changes

rtkaczyk force-pushed the etcm-76-checkpoint-network-messages branch from 024513c to 0423f35 Compare October 28, 2020 09:48

[ETCM-76] network messages for checkpointing

f0befc1

rtkaczyk force-pushed the etcm-76-checkpoint-network-messages branch from 0423f35 to f0befc1 Compare October 28, 2020 11:11

rtkaczyk merged commit f0befc1 into develop Oct 28, 2020

rtkaczyk deleted the etcm-76-checkpoint-network-messages branch October 28, 2020 12:48

Conversation

rtkaczyk commented Oct 9, 2020

Description

Uh oh!

pslaski left a comment

Choose a reason for hiding this comment

Uh oh!

pslaski Oct 22, 2020

Choose a reason for hiding this comment

Uh oh!

pslaski Oct 22, 2020

Choose a reason for hiding this comment

Uh oh!

rtkaczyk Oct 23, 2020

Choose a reason for hiding this comment

Uh oh!

pslaski Oct 26, 2020

Choose a reason for hiding this comment

Uh oh!

rtkaczyk Oct 27, 2020

Choose a reason for hiding this comment

Uh oh!

pslaski Oct 22, 2020

Choose a reason for hiding this comment

Uh oh!

ntallar left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ntallar commented Oct 23, 2020

Uh oh!

rtkaczyk commented Oct 23, 2020

Uh oh!

ntallar commented Oct 23, 2020

Uh oh!

rtkaczyk commented Oct 23, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ntallar commented Oct 23, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ntallar left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ntallar commented Oct 26, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ntallar left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

rtkaczyk commented Oct 23, 2020 •

edited

Loading

ntallar commented Oct 23, 2020 •

edited

Loading

ntallar commented Oct 26, 2020 •

edited

Loading

ntallar left a comment •

edited

Loading