[ETCM-841] Rework protocol negotiation #1004
Conversation
dzajkowski
force-pushed
the
feature/etcm-841-protocol-capabilities-config-reorg
branch
2 times, most recently
from
539fc60 to
85e6894
Compare
src/main/scala/io/iohk/ethereum/network/handshaker/EtcHelloExchangeState.scala
Outdated
Show resolved
Hide resolved
src/main/scala/io/iohk/ethereum/network/p2p/messages/Capability.scala
Outdated
Show resolved
Hide resolved
dzajkowski
force-pushed
the
feature/etcm-841-protocol-capabilities-config-reorg
branch
from
ca927e5 to
99dc3b4
Compare
|
|
||
| private val pattern = "(.*)/(\\d*)".r | ||
|
|
||
| def from(protocolVersion: String): Capability = |
There was a problem hiding this comment.
Could we make it clear that this function throws ? parseUnsafe maybe ?
Or make it return an option and let BlockchainConfig throw in case of none
There was a problem hiding this comment.
good point. I'll stick to the exception. not the best solution but we need mantis to fail when configs are not properly defined.
| @@ -246,7 +248,7 @@ trait PeerManagerActorBuilder { | |||
| EthereumMessageDecoder, | |||
| discoveryConfig, | |||
| blacklist, | |||
| Config.Network.protocolVersion | |||
| blockchainConfig.capabilities // TODO replace with a list of capabilities | |||
There was a problem hiding this comment.
Isn't it already a list of capabilities ?
| @@ -122,6 +124,8 @@ object BlockchainConfig { | |||
| val allowedMinersPublicKeys = readPubKeySet(blockchainConfig, "allowed-miners") | |||
|
|
|||
| val ecip1099BlockNumber: BigInt = BigInt(blockchainConfig.getString("ecip1099-block-number")) | |||
| val capabilities: List[Capability] = | |||
| blockchainConfig.getStringList("capabilities").asScala.toList.map(Capability.from) | |||
There was a problem hiding this comment.
I guess we should also check that the capability in the config is indeed a capability implemented by Mantis ?
| @@ -7,6 +7,21 @@ import io.iohk.ethereum.rlp.{RLPEncodeable, RLPException, RLPList, RLPSerializab | |||
| case class Capability(name: String, version: Byte) | |||
|
|
|||
| object Capability { | |||
| def negotiate(c1: List[Capability], c2: List[Capability]): Option[Capability] = | |||
| c1.intersect(c2).maxByOption(_.version) // FIXME ignores branches and other protocols | |||
There was a problem hiding this comment.
It seems that it duplicates the best function here.
We should probably have a single point where the best protocol is chosen, no ?
Something like :
| c1.intersect(c2).maxByOption(_.version) // FIXME ignores branches and other protocols | |
| // FIXME ignores branches and other protocols | |
| c1.intersect(c2) match { | |
| case Nil => None | |
| case nonEmpty => best(nonEmpty) | |
| } |
|
The ETCM-841 ticket description mentions "The task is to rename the whole protocol to PV164 and mark @depracated". Are you thinking keeping it as ETC/64 instead? With or without deprecation? |
dzajkowski
force-pushed
the
feature/etcm-841-protocol-capabilities-config-reorg
branch
from
1785b82 to
8648b65
Compare
|
@LeonorLunatech As I mentioned during refinement - I went through a few iterations on how to deal with this issue and by explicitly stating the namespace (prefix) mantis can have any number of protocols. The only limitation will be the negotiation mechanism. TL;DR the deprecation is not needed with the prefix. This way mantis stays backwards compatible. |
dzajkowski
deleted the
feature/etcm-841-protocol-capabilities-config-reorg
branch
Mantis implements a protocol version ETC/64 which is conflicting with ETH/64.
This PRs goal is to enable the coexistence of both and some future proofing (renames, simple protocol negotiation).