♻️ 🧹 Removed Stop from GetBlockHashesMsg

[greymistcube]

• Removed Stop from GetBlockHashesMsg. I suspect this was implemented to imitate the Bitcoin's implementation. I don't really see a rationale for this to exist.
• Cleaned up BlockLocator and GetBlockHashesMsg accordingly.

Addendum

• Partially reverted GetBlockHashesMsg encoding to be backward compatible as a network protocol.

[riemannulus]

Is it backward compatible?

[greymistcube]

The backward compatibility has been broken since #3913, albeit on a different level. This further(?) breaks the compatibility since it has been already broken. 🙄

[greymistcube]

The result is the same, that is, any node running with a different Libplanet version would not be able to sync to a node running the newest version.

[riemannulus]

So is there any way to test a new version of Libplanet on a network where the live version is running?

[greymistcube]

It is possible to apply only the business logic and force partial compatibility as I have with #3913, but I don't think that is necessary (or worth the time). If you are still worried, we can further divide this PR into multiple releases where

• We keep partial compatibility.
  • Message level compatibility is left intact.
  • Ignore BlockLocators with multiple BlockHashes and/or non-null Stop.
  • This results in a newer node being able to sync to an old node, but not the other way around.
  • This only breaks the business logic layer.
• We break compatibility completely by cleaning up/updating the GetBlockHashesMsg encoding.
  • This would result in a newer node and an old node not being able to sync to each other.
  • This breaks the network layer as well.

As the newer node is about getting rid of "unnecessary" block sync logic and dependencies, the compatibility has to be broken at some point.

[riemannulus]

I don't think backward compatibility is necessary on this. But, at least, we need to test this feature on our dev cluster.

[greymistcube]

Arg, might as well separate the encoding part into a separate PR for easier testing. 😕

[greymistcube]

@riemannulus @limebell
Added ce6e275 for backwards compatibility.
Funnily enough, this is more compatible with old versions of Libplanet than after #3913.
A newer node would simply ignore irrelevant information given and interpret GetBlockHashesMsg differently
(and behave accordingly).
An outgoing GetBlockHashesMsg from a newer node can also be read normally by older nodes.

[limebell]

Overall logic looks good to me, are you plan to change MessageType in other PR?

[greymistcube]

@limebell

Overall logic looks good to me, are you plan to change MessageType in other PR?

If we leave as it is without changing the encoding/decoding process, this should be compatible with the old MessageType.

• A newer version only sends out GetBlockHashesMsg with a single BlockHash in a compliant form which can be interpreted by an older version.
• A newer version can parse a GetBlockHashesMsg (although this is interpreted differently) sent by an older version.

I will update it when we change the encoding.