-
Notifications
You must be signed in to change notification settings - Fork 78
[ETCM-354] Peer fork ID validation #1024
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
762ca2b
ac09b48
76488cf
ab06959
3a10074
39eeae5
74b8f75
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,139 @@ | ||
| package io.iohk.ethereum.forkid | ||
|
|
||
| import akka.util.ByteString | ||
| import cats.Monad | ||
| import cats.data.EitherT._ | ||
| import cats.implicits._ | ||
| import io.iohk.ethereum.utils.BigIntExtensionMethods._ | ||
| import io.iohk.ethereum.utils.BlockchainConfig | ||
| import io.iohk.ethereum.utils.ByteUtils._ | ||
| import monix.eval.Task | ||
| import org.typelevel.log4cats.Logger | ||
| import org.typelevel.log4cats.slf4j.Slf4jLogger | ||
|
|
||
| import java.util.zip.CRC32 | ||
|
|
||
| sealed trait ForkIdValidationResult | ||
| case object Connect extends ForkIdValidationResult | ||
| case object ErrRemoteStale extends ForkIdValidationResult | ||
| case object ErrLocalIncompatibleOrStale extends ForkIdValidationResult | ||
|
|
||
| object ForkIdValidator { | ||
|
|
||
| implicit val unsafeLogger = Slf4jLogger.getLogger[Task] | ||
|
|
||
| val maxUInt64 = (BigInt(0x7fffffffffffffffL) << 1) + 1 // scalastyle:ignore magic.number | ||
|
|
||
| /** Tells whether it makes sense to connect to a peer or gives a reason why it isn't a good idea. | ||
| * | ||
| * @param genesisHash - hash of the genesis block of the current chain | ||
| * @param config - local client's blockchain configuration | ||
| * @param currentHeight - number of the block at the current tip | ||
| * @param remoteId - ForkId announced by the connecting peer | ||
| * @return One of: | ||
| * - [[io.iohk.ethereum.forkid.Connect]] - It is safe to connect to the peer | ||
| * - [[io.iohk.ethereum.forkid.ErrRemoteStale]] - Remote is stale, don't connect | ||
| * - [[io.iohk.ethereum.forkid.ErrLocalIncompatibleOrStale]] - Local is incompatible or stale, don't connect | ||
| */ | ||
| def validatePeer[F[_]: Monad: Logger]( | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could do with a scaladoc since it's 'external' API, right?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good catch, added
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we really need the monad ?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
| genesisHash: ByteString, | ||
| config: BlockchainConfig | ||
| )(currentHeight: BigInt, remoteForkId: ForkId): F[ForkIdValidationResult] = { | ||
| val forks = ForkId.gatherForks(config) | ||
| validatePeer[F](genesisHash, forks)(currentHeight, remoteForkId) | ||
| } | ||
|
|
||
| private[forkid] def validatePeer[F[_]: Monad: Logger]( | ||
| genesisHash: ByteString, | ||
| forks: List[BigInt] | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think this is necessary |
||
| )(currentHeight: BigInt, remoteId: ForkId): F[ForkIdValidationResult] = { | ||
| val checksums: Vector[BigInt] = calculateChecksums(genesisHash, forks) | ||
|
|
||
| // find the first unpassed fork and it's index | ||
| val (unpassedFork, unpassedForkIndex) = | ||
| 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 { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think I can see what you mean. Could you paste a code snippet with the suggested simplification? |
||
| _ <- liftF(Logger[F].trace(s"Before checkMatchingHashes")) | ||
| matching <- fromEither[F]( | ||
| checkMatchingHashes(checksums(unpassedForkIndex), remoteId, currentHeight).toLeft("hashes didn't match") | ||
| ) | ||
| _ <- liftF(Logger[F].trace(s"checkMatchingHashes result: $matching")) | ||
| _ <- liftF(Logger[F].trace(s"Before checkSubset")) | ||
| sub <- fromEither[F](checkSubset(checksums, forks, remoteId, unpassedForkIndex).toLeft("not in subset")) | ||
| _ <- liftF(Logger[F].trace(s"checkSubset result: $sub")) | ||
| _ <- liftF(Logger[F].trace(s"Before checkSuperset")) | ||
| sup <- fromEither[F](checkSuperset(checksums, remoteId, unpassedForkIndex).toLeft("not in superset")) | ||
| _ <- liftF(Logger[F].trace(s"checkSuperset result: $sup")) | ||
| _ <- liftF(Logger[F].trace(s"No check succeeded")) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we could do with just one log between each step right ? |
||
| _ <- fromEither[F](Either.left[ForkIdValidationResult, Unit](ErrLocalIncompatibleOrStale)) | ||
| } yield ()).value | ||
|
|
||
| for { | ||
| _ <- Logger[F].debug(s"Validating $remoteId") | ||
| _ <- Logger[F].trace(s" list: $forks") | ||
| _ <- Logger[F].trace(s"Unpassed fork $unpassedFork was found at index $unpassedForkIndex") | ||
| res <- validate.map(_.swap) | ||
| _ <- Logger[F].debug(s"Validation result is: $res") | ||
| } yield (res.getOrElse(Connect)) | ||
| } | ||
|
|
||
| private def calculateChecksums( | ||
| genesisHash: ByteString, | ||
| forks: List[BigInt] | ||
| ): Vector[BigInt] = { | ||
| val crc = new CRC32() | ||
| crc.update(genesisHash.asByteBuffer) | ||
| val genesisChecksum = BigInt(crc.getValue()) | ||
|
|
||
| genesisChecksum +: (forks.map { fork => | ||
| crc.update(bigIntToBytes(fork, 8)) | ||
| BigInt(crc.getValue()) | ||
| }).toVector | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 (But this is just a suggestion ofc.)
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This code isn't functional because of a side effect in |
||
| } | ||
|
|
||
| /** | ||
| * 1) If local and remote FORK_HASH matches, compare local head to FORK_NEXT. | ||
| * The two nodes are in the same fork state currently. | ||
| * They might know of differing future forks, but that’s not relevant until the fork triggers (might be postponed, nodes might be updated to match). | ||
| * 1a) A remotely announced but remotely not passed block is already passed locally, disconnect, since the chains are incompatible. | ||
| * 1b) No remotely announced fork; or not yet passed locally, connect. | ||
| */ | ||
| private def checkMatchingHashes( | ||
| checksum: BigInt, | ||
| remoteId: ForkId, | ||
| currentHeight: BigInt | ||
| ): Option[ForkIdValidationResult] = | ||
| remoteId match { | ||
| case ForkId(hash, _) if checksum != hash => None | ||
| case ForkId(_, Some(next)) if currentHeight >= next => Some(ErrLocalIncompatibleOrStale) | ||
| case _ => Some(Connect) | ||
| } | ||
|
|
||
| /** | ||
| * 2) If the remote FORK_HASH is a subset of the local past forks and the remote FORK_NEXT matches with the locally following fork block number, connect. | ||
| * Remote node is currently syncing. It might eventually diverge from us, but at this current point in time we don’t have enough information. | ||
| */ | ||
| def checkSubset( | ||
| checksums: Vector[BigInt], | ||
| forks: List[BigInt], | ||
| remoteId: ForkId, | ||
| i: Int | ||
| ): Option[ForkIdValidationResult] = | ||
| checksums | ||
| .zip(forks) | ||
| .take(i) | ||
| .collectFirst { | ||
| case (sum, fork) if sum == remoteId.hash => if (fork == remoteId.next.getOrElse(0)) Connect else ErrRemoteStale | ||
| } | ||
|
|
||
| /** | ||
| * 3) If the remote FORK_HASH is a superset of the local past forks and can be completed with locally known future forks, connect. | ||
| * Local node is currently syncing. It might eventually diverge from the remote, but at this current point in time we don’t have enough information. | ||
| */ | ||
| def checkSuperset(checksums: Vector[BigInt], remoteId: ForkId, i: Int): Option[ForkIdValidationResult] = { | ||
| checksums.drop(i).collectFirst { case sum if sum == remoteId.hash => Connect } | ||
| } | ||
|
|
||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,102 @@ | ||
| package io.iohk.ethereum.forkid | ||
|
|
||
| import akka.util.ByteString | ||
| import io.iohk.ethereum.forkid.ForkId._ | ||
| import io.iohk.ethereum.utils.Config._ | ||
| import io.iohk.ethereum.utils.ForkBlockNumbers | ||
| import monix.eval.Task | ||
| import monix.execution.Scheduler.Implicits.global | ||
| import org.bouncycastle.util.encoders.Hex | ||
| import org.scalatest.matchers.should._ | ||
| import org.scalatest.wordspec.AnyWordSpec | ||
|
|
||
| import scala.concurrent.duration._ | ||
|
|
||
| import ForkIdValidator._ | ||
|
|
||
| class ForkIdValidatorSpec extends AnyWordSpec with Matchers { | ||
|
|
||
| val config = blockchains | ||
|
|
||
| val ethGenesisHash = ByteString(Hex.decode("d4e56740f876aef8c010b86a40d5f56745a118d0906a34e69aec8c0db1cb8fa3")) | ||
|
|
||
| "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) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. now I'm curious - how does one property test an equation? by writing the same equation in a different way?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. oh this isn't arbitrary data - it's hard fork heights.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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? |
||
|
|
||
| def validatePeer(head: BigInt, remoteForkId: ForkId) = | ||
| ForkIdValidator | ||
| .validatePeer[Task](ethGenesisHash, ethForksList)(head, remoteForkId) | ||
| .runSyncUnsafe(Duration(1, SECONDS)) | ||
|
|
||
| // Local is mainnet Petersburg, remote announces the same. No future fork is announced. | ||
| validatePeer(7987396, ForkId(0x668db0afL, None)) shouldBe Connect | ||
|
|
||
| // Local is mainnet Petersburg, remote announces the same. Remote also announces a next fork | ||
| // at block 0xffffffff, but that is uncertain. | ||
| validatePeer(7279999, ForkId(0xa00bc324L, Some(ForkIdValidator.maxUInt64))) shouldBe Connect | ||
|
|
||
| // Local is mainnet currently in Byzantium only (so it's aware of Petersburg), remote announces | ||
| // also Byzantium, and it's also aware of Petersburg (e.g. updated node before the fork). We | ||
| // don't know if Petersburg passed yet (will pass) or not. | ||
| validatePeer(7279999, ForkId(0xa00bc324L, Some(7280000))) shouldBe Connect | ||
|
|
||
| // Local is mainnet Petersburg, remote announces the same. Remote also announces a next fork | ||
| // at block 0xffffffff, but that is uncertain. | ||
| validatePeer(7987396, ForkId(0x668db0afL, Some(ForkIdValidator.maxUInt64))) shouldBe Connect | ||
|
|
||
| // Local is mainnet currently in Byzantium only (so it's aware of Petersburg), remote announces | ||
| // also Byzantium, but it's not yet aware of Petersburg (e.g. non updated node before the fork). | ||
| // In this case we don't know if Petersburg passed yet or not. | ||
| validatePeer(7279999, ForkId(0xa00bc324L, None)) shouldBe Connect | ||
|
|
||
| validatePeer(7279999, ForkId(0xa00bc324L, Some(7280000))) shouldBe Connect | ||
|
|
||
| // Local is mainnet currently in Byzantium only (so it's aware of Petersburg), remote announces | ||
| // also Byzantium, and it's also aware of some random fork (e.g. misconfigured Petersburg). As | ||
| // neither forks passed at neither nodes, they may mismatch, but we still connect for now. | ||
| validatePeer(7279999, ForkId(0xa00bc324L, Some(ForkIdValidator.maxUInt64))) shouldBe Connect | ||
|
|
||
| // Local is mainnet Petersburg, remote announces Byzantium + knowledge about Petersburg. Remote | ||
| // is simply out of sync, accept. | ||
| validatePeer(7987396, ForkId(0xa00bc324L, Some(7280000))) shouldBe Connect | ||
|
|
||
| // Local is mainnet Petersburg, remote announces Spurious + knowledge about Byzantium. Remote | ||
| // is definitely out of sync. It may or may not need the Petersburg update, we don't know yet. | ||
| validatePeer(7987396, ForkId(0x3edd5b10L, Some(4370000))) shouldBe Connect | ||
|
|
||
| // Local is mainnet Byzantium, remote announces Petersburg. Local is out of sync, accept. | ||
| validatePeer(7279999, ForkId(0x668db0afL, None)) shouldBe Connect | ||
|
|
||
| // Local is mainnet Spurious, remote announces Byzantium, but is not aware of Petersburg. Local | ||
| // out of sync. Local also knows about a future fork, but that is uncertain yet. | ||
| validatePeer(4369999, ForkId(0xa00bc324L, None)) shouldBe Connect | ||
|
|
||
| // Local is mainnet Petersburg. remote announces Byzantium but is not aware of further forks. | ||
| // Remote needs software update. | ||
| validatePeer(7987396, ForkId(0xa00bc324L, None)) shouldBe ErrRemoteStale | ||
|
|
||
| // Local is mainnet Petersburg, and isn't aware of more forks. Remote announces Petersburg + | ||
| // 0xffffffff. Local needs software update, reject. | ||
| validatePeer(7987396, ForkId(0x5cddc0e1L, None)) shouldBe ErrLocalIncompatibleOrStale | ||
|
|
||
| // Local is mainnet Byzantium, and is aware of Petersburg. Remote announces Petersburg + | ||
| // 0xffffffff. Local needs software update, reject. | ||
| validatePeer(7279999, ForkId(0x5cddc0e1L, None)) shouldBe ErrLocalIncompatibleOrStale | ||
|
|
||
| // Local is mainnet Petersburg, remote is Rinkeby Petersburg. | ||
| validatePeer(7987396, ForkId(0xafec6b27L, None)) shouldBe ErrLocalIncompatibleOrStale | ||
|
|
||
| // Local is mainnet Petersburg, far in the future. Remote announces Gopherium (non existing fork) | ||
| // at some future block 88888888, for itself, but past block for local. Local is incompatible. | ||
| // | ||
| // This case detects non-upgraded nodes with majority hash power (typical Ropsten mess). | ||
| validatePeer(88888888, ForkId(0x668db0afL, Some(88888888))) shouldBe ErrLocalIncompatibleOrStale | ||
|
|
||
| // Local is mainnet Byzantium. Remote is also in Byzantium, but announces Gopherium (non existing | ||
| // fork) at block 7279999, before Petersburg. Local is incompatible. | ||
| validatePeer(7279999, ForkId(0xa00bc324L, Some(7279999L))) shouldBe ErrLocalIncompatibleOrStale | ||
| } | ||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we really need to add 3 deps for one class ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sorry, it's only 2 my bad
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we have a limit for that? :-)