[ETCM-354] Peer fork ID validation by lukasz-golebiewski · Pull Request #1024 · input-output-hk/mantis

lukasz-golebiewski · 2021-06-23T14:25:06Z

Description

Adds peer fork id validation as described in the section "validation rules" of https://eips.ethereum.org/EIPS/eip-2124

jvdp

Looks OK to my eyeballs but the code and tests could be simpler. And it will be more interesting once this is hooked into handshaking and running on a test net.

jvdp · 2021-06-24T10:13:17Z

+    val (unpassedFork, i) = (forks :+ maxUInt64).zipWithIndex.find { case (fork, _) => currentHeight < fork }.get
+
+    // The checks are left biased -> whenever a result is found we need to short circuit
+    val validate: F[Option[ForkIdValidationResult]] = (for {


This method looks a lot more complicated than it is. How about moving the tracing into the check methods, for a start? (Do we still need the tracing btw?)

This method looks a lot more complicated than it is.

This is vague, can you elaborate on that?

How about moving the tracing into the check methods, for a start?

It could be done, but I can't see how moving logging instructions around is going to make the method less complicated.

(Do we still need the tracing btw?)

Logging is part of our DoD and it might get useful when testing on testnet, so I wouldn't remove it.

jvdp · 2021-06-24T10:14:09Z

+      res <- fromEither[F](Either.left[ForkIdValidationResult, Unit](ErrLocalIncompatibleOrStale))
+    } yield (res)).value
+      .map(_.swap)
+      .flatMap(res => Logger[F].debug(s"Validation result is: $res") >> Monad[F].pure(res.toOption))


Could also use a for-comprehension for this part? (And bind the existing one to a name.)

jvdp · 2021-06-24T10:14:56Z

+
+  val maxUInt64 = (BigInt(0x7fffffffffffffffL) << 1) + 1 // scalastyle:ignore magic.number
+
+  def validatePeer[F[_]: Monad: Logger](


Could do with a scaladoc since it's 'external' API, right?

Good catch, added

Do we really need the monad ?
This is a synchronous function, so there is no need for an async abstraction. I get that logging is a kind of side effect, but from the caller point of view it does not really matter, and it adds some noise because everything needs to be inside a liftF now.

Monad has nothing to do with asynchronicity, it allows chaining effectful computations.
Logging is a side effect. Are you saying we should ignore that fact?

jvdp · 2021-06-24T10:17:16Z

+    genesisChecksum +: (forks.map { fork =>
+      crc.update(bigIntToBytes(fork, 8))
+      BigInt(crc.getValue())
+    }).toVector


This code perhaps looks functional but it's really relying on the mutable CRC instance. It's probably more performant this way, but it could be more descriptive using List.unfold or similar. (Eg. first produce a collection of collections to produce the CRC with, then flatten it.)

(But this is just a suggestion ofc.)

This code isn't functional because of a side effect in map. It is because of the nature of Java's CRC as you correctly noted. This however doesn't break the referential transparency of calculateChecksum, so the function itself remains pure.

jvdp · 2021-06-24T10:27:45Z

+        Some(Connect)
+      }
+    } else { None }
+  }


Could be nicer as a match on remoteId:

remoteId match { case ForkId(hash, _) if checksums(i) != hash => None case ForkId(_, Some(next)) if currentHeight >= next => Some(ErrLocalIncompatibleOrStale) case _ => Some(Connect) }

(Gets rid of the nested if and the Option.get.)

jvdp · 2021-06-24T10:33:56Z

+  "ForkIdValidator" must {
+    "correctly validate ETH peers" in {
+      // latest fork at the time of writing those assertions (in the spec) was Petersburg
+      val ethForksList: List[BigInt] = List(1150000, 1920000, 2463000, 2675000, 4370000, 7280000)


Are all these different forks interesting for the purposes of testing? I mean, you are testing it with 6, would it make a material difference to the test quality if it was 5 or 7 or 9000?

Also, would this be a good opportunity for property based testing perhaps?

now I'm curious - how does one property test an equation? by writing the same equation in a different way?

Yes, but part of this 'equation' would be in how the arbitrary data is constructed. I agree that just re-implementing the same logic wouldn't make much sense.

oh this isn't arbitrary data - it's hard fork heights.

This test was provided as part of the specification in https://eips.ethereum.org/EIPS/eip-2124 so we would have to have a really good reason to modify or not include it here.

Whether all the forks are interesting for the purposes of testing is probably a question to the original EIP's authors.

@jvdp what are the properties you would like to test here?

leo-bogastry · 2021-06-24T12:01:53Z

+
+  def validatePeer[F[_]: Monad: Logger](
+      genesisHash: ByteString,
+      forks: List[BigInt]


Should forks be validated to not be empty?

I don't think this is necessary

AurelienRichez · 2021-06-24T15:52:32Z

-    "org.codehaus.janino" % "janino" % "3.1.2"
+    "org.codehaus.janino" % "janino" % "3.1.2",
+    "org.typelevel" %% "log4cats-core" % "2.1.1",
+    "org.typelevel" %% "log4cats-slf4j" % "1.3.1"


do we really need to add 3 deps for one class ?

sorry, it's only 2 my bad

Do we have a limit for that? :-)

AurelienRichez · 2021-06-25T07:55:14Z

+      _ <- liftF(Logger[F].trace(s"Before checkSuperset"))
+      sup <- fromEither[F](checkSuperset(checksums, remoteId, i).toLeft("not in superset"))
+      _ <- liftF(Logger[F].trace(s"checkSuperset result: $sup"))
+      _ <- liftF(Logger[F].trace(s"No check succeeded"))


I think we could do with just one log between each step right ?

AurelienRichez · 2021-06-25T08:10:22Z

+      checksums: Vector[BigInt],
+      remoteId: ForkId,
+      currentHeight: BigInt,
+      i: Int


we could directly pass the current fork block hash instead of i and a list. It's not really obvious that the index of the next fork is actually also the index of the current checksums.

AurelienRichez · 2021-06-25T08:26:55Z

+      forks.zipWithIndex.find { case (fork, _) => currentHeight < fork }.getOrElse((maxUInt64, forks.length))
+
+    // The checks are left biased -> whenever a result is found we need to short circuit
+    val validate = (for {


can't we find the hash in checksums which match remote hash first ? So that we directly know if we should do the matching/subset/superset check directly ? I feel that it would simplify the function.

I don't think I can see what you mean. Could you paste a code snippet with the suggested simplification?

AurelienRichez · 2021-06-25T08:28:21Z

+    val checksums: Vector[BigInt] = calculateCheckchecksums(genesisHash, forks)
+
+    // find the first unpassed fork and it's index
+    val (unpassedFork, i) =


i -> nextForkIndex or unpassedForkIndex ?

AurelienRichez · 2021-06-25T08:40:05Z

+    } yield (res.getOrElse(Connect))
+  }
+
+  private def calculateCheckchecksums(


typo calculateChecksums

AurelienRichez · 2021-06-25T08:47:57Z

+
+  val maxUInt64 = (BigInt(0x7fffffffffffffffL) << 1) + 1 // scalastyle:ignore magic.number
+
+  def validatePeer[F[_]: Monad: Logger](


Do we really need the monad ?
This is a synchronous function, so there is no need for an async abstraction. I get that logging is a kind of side effect, but from the caller point of view it does not really matter, and it adds some noise because everything needs to be inside a liftF now.

dzajkowski

I created a set of topics in slack to continue the discussion but from my pov it LGTM

lukasz-golebiewski force-pushed the feature/etcm-354/forkid-check branch 2 times, most recently from 423bc93 to 10cdc43 Compare June 23, 2021 14:47

dzajkowski reviewed Jun 23, 2021

View reviewed changes

Comment thread src/main/scala/io/iohk/ethereum/forkid/ForkIdValidator.scala Outdated

lukasz-golebiewski force-pushed the feature/etcm-354/forkid-check branch 2 times, most recently from 476bc61 to 7202b88 Compare June 24, 2021 09:14

jvdp approved these changes Jun 24, 2021

View reviewed changes

leo-bogastry reviewed Jun 24, 2021

View reviewed changes

Comment thread src/main/scala/io/iohk/ethereum/forkid/ForkIdValidator.scala Outdated

lukasz-golebiewski added 4 commits June 24, 2021 15:17

[ETCM-354] Peer fork ID validation

762ca2b

[ETCM-354] Apply review suggestions

ac09b48

[ETCM-354] Update sbt sha

76488cf

[ETCM-354] Add scaladoc for validatePeer

ab06959

lukasz-golebiewski force-pushed the feature/etcm-354/forkid-check branch from a32e329 to 1050ee8 Compare June 24, 2021 13:17

[ETCM-354] Implement another round of review suggestions

3a10074

lukasz-golebiewski force-pushed the feature/etcm-354/forkid-check branch from 1050ee8 to 3a10074 Compare June 24, 2021 13:45

leo-bogastry approved these changes Jun 25, 2021

View reviewed changes

AurelienRichez reviewed Jun 25, 2021

View reviewed changes

lukasz-golebiewski added 2 commits June 25, 2021 13:57

[ETCM-354] Fix typo

39eeae5

[ETCM-354] More review suggestions

74b8f75

dzajkowski approved these changes Jun 25, 2021

View reviewed changes

lukasz-golebiewski merged commit 127524c into develop Jun 25, 2021

lukasz-golebiewski deleted the feature/etcm-354/forkid-check branch June 25, 2021 13:13


		val maxUInt64 = (BigInt(0x7fffffffffffffffL) << 1) + 1 // scalastyle:ignore magic.number

		def validatePeer[F[_]: Monad: Logger](

Conversation

lukasz-golebiewski commented Jun 23, 2021

Description

Uh oh!

Uh oh!

jvdp left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lukasz-golebiewski Jun 25, 2021 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dzajkowski left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

lukasz-golebiewski Jun 25, 2021 •

edited

Loading