diff --git a/.github/workflows/check-pr.yml b/.github/workflows/check-pr.yml index 7328f1de..a30f84e1 100644 --- a/.github/workflows/check-pr.yml +++ b/.github/workflows/check-pr.yml @@ -16,7 +16,6 @@ jobs: timeout-minutes: 45 env: JAVA_OPTS: -Dfile.encoding=UTF-8 - BRANCH_NAME: ${{ github.head_ref || github.ref_name }} steps: - uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2 - uses: actions/setup-java@be666c2fcd27ec809703dec50e508c2fdc7f6654 # v5.2.0 @@ -40,7 +39,7 @@ jobs: uses: actions/upload-artifact@v4 if: always() with: - name: test-logs_${{ env.BRANCH_NAME }}_${{ github.run_id }} + name: test-logs_${{ github.run_id }}_${{ github.run_attempt }} path: consensus-client-it/target/test-logs if-no-files-found: warn retention-days: 14 diff --git a/consensus-client-it/src/test/scala/units/BaseDockerTestSuite.scala b/consensus-client-it/src/test/scala/units/BaseDockerTestSuite.scala index 474b92a7..71bbcd31 100644 --- a/consensus-client-it/src/test/scala/units/BaseDockerTestSuite.scala +++ b/consensus-client-it/src/test/scala/units/BaseDockerTestSuite.scala @@ -21,6 +21,8 @@ import units.el.{NativeBridgeClient, StandardBridgeClient} import units.eth.{EthAddress, Gwei} import units.test.{CustomMatchers, IntegrationTestEventually, TestEnvironment} +import java.nio.channels.FileChannel +import java.nio.file.{Path, StandardOpenOption} import scala.sys.process.{Process, ProcessLogger} trait BaseDockerTestSuite @@ -117,11 +119,14 @@ trait BaseDockerTestSuite protected def deploySolidityContracts(): Unit = { step("Deploy contracts on EL") - Process( - s"forge script -vvvvv scripts/IT.s.sol:IT --private-key $elRichAccount1PrivateKey --fork-url http://localhost:${ec1.rpcPort} --broadcast", - TestEnvironment.ContractsDir, - "CHAIN_ID" -> EcContainer.ChainId.toString - ).!(ProcessLogger(out => log.info(out), err => log.error(err))) + val exitCode = BaseDockerTestSuite.withForgeLock { + Process( + s"forge script -vvvvv scripts/IT.s.sol:IT --private-key $elRichAccount1PrivateKey --fork-url http://localhost:${ec1.rpcPort} --broadcast", + TestEnvironment.ContractsDir, + "CHAIN_ID" -> EcContainer.ChainId.toString + ).!(ProcessLogger(out => log.info(out), err => log.error(err))) + } + withClue("forge script exit code:")(exitCode shouldBe 0) waitForContract(StandardBridgeAddress) waitForContract(WWavesAddress) @@ -154,6 +159,20 @@ trait BaseDockerTestSuite object BaseDockerTestSuite { private val initialized = AtomicBoolean(false) + private val ForgeLockFile = Path.of(System.getProperty("java.io.tmpdir"), "consensus-client-it-forge.lock") + + /** forge script keeps its broadcast state per script and chain id in the shared contracts cache directory and refuses to run while another process + * modifies it. Suites run in parallel JVMs with the same chain id, so deployments are serialized with an OS-level file lock. + */ + def withForgeLock[A](f: => A): A = { + val channel = FileChannel.open(ForgeLockFile, StandardOpenOption.CREATE, StandardOpenOption.WRITE) + try { + val lock = channel.lock() + try f + finally lock.release() + } finally channel.close() + } + def init(): Unit = if (initialized.compareAndSet(expect = false, update = true)) AddressScheme.current = new AddressScheme { diff --git a/consensus-client-it/src/test/scala/units/ForkchoiceNoFinalizedBlockTestSuite.scala b/consensus-client-it/src/test/scala/units/ForkchoiceNoFinalizedBlockTestSuite.scala new file mode 100644 index 00000000..0c33d942 --- /dev/null +++ b/consensus-client-it/src/test/scala/units/ForkchoiceNoFinalizedBlockTestSuite.scala @@ -0,0 +1,82 @@ +package units + +import com.wavesplatform.common.utils.EitherExt2.explicitGet +import org.web3j.protocol.core.DefaultBlockParameterName +import units.ELUpdater.NoFinalizedBlockHash +import units.client.engine.model.{BlockNumber, EcBlock} +import units.eth.EthAddress + +/** The consensus client reports no finalized block (an all-zero hash) in forkchoiceUpdated when the new head is below the contract's finalized block. + * Checks that op-geth accepts it, moves the head, and keeps the previous finalized block. + * + * Drives op-geth directly through the engine API, so only the EL node is started. + */ +class ForkchoiceNoFinalizedBlockTestSuite extends BaseDockerTestSuite { + private val prevRandao = "0x" + "0" * 64 + private val minerA = EthAddress.unsafeFrom("0x" + "a" * 40) + private val minerB = EthAddress.unsafeFrom("0x" + "b" * 40) + + /** Builds a block on top of the current head `parent` and imports it without making it the head. */ + private def buildBlock(parent: EcBlock, finalized: BlockHash, feeRecipient: EthAddress): EcBlock = { + val payloadId = ec1.engineApi + .forkchoiceUpdatedWithPayload(parent.hash, finalized, parent.timestamp + 1, feeRecipient, prevRandao, Vector.empty, Vector.empty) + .explicitGet() + val payload = ec1.engineApi.getPayload(payloadId).explicitGet() + val hash = ec1.engineApi.newPayload(payload).explicitGet().value + ec1.engineApi.getBlockByHash(hash).explicitGet().value + } + + private def head: BlockHash = ec1.engineApi.getLastExecutionBlock().explicitGet().hash + + private def finalizedHash: Option[BlockHash] = { + val r = ec1.web3j.ethGetBlockByNumber(DefaultBlockParameterName.FINALIZED, false).send() + Option.when(!r.hasError)(r.getBlock).flatMap(Option(_)).map(b => BlockHash(b.getHash)) + } + + "forkchoiceUpdated with no finalized block" in { + val genesis = ec1.engineApi.getBlockByNumber(BlockNumber.Number(0)).explicitGet().value + + step("Build a1 on genesis with no finalized block") + val a1 = buildBlock(genesis, NoFinalizedBlockHash, minerA) + + step("Make a1 the head with no finalized block") + ec1.engineApi.forkchoiceUpdated(a1.hash, NoFinalizedBlockHash).explicitGet() + head shouldBe a1.hash + finalizedHash shouldBe empty + + step("Build siblings a2 and b2 on a1") + val a2 = buildBlock(a1, NoFinalizedBlockHash, minerA) + val b2 = buildBlock(a1, NoFinalizedBlockHash, minerB) + a2.hash should not be b2.hash + + step("Make a2 the head, a1 finalized") + ec1.engineApi.forkchoiceUpdated(a2.hash, a1.hash).explicitGet() + head shouldBe a2.hash + finalizedHash.value shouldBe a1.hash + + step("Switch the head to the sibling b2 with no finalized block: accepted, a1 stays finalized") + ec1.engineApi.forkchoiceUpdated(b2.hash, NoFinalizedBlockHash).explicitGet() + head shouldBe b2.hash + finalizedHash.value shouldBe a1.hash + + step("Switch back to a2 with no finalized block") + ec1.engineApi.forkchoiceUpdated(a2.hash, NoFinalizedBlockHash).explicitGet() + head shouldBe a2.hash + finalizedHash.value shouldBe a1.hash + + step("A finalized block that isn't on the head's chain is rejected, the finalized block doesn't change") + ec1.engineApi.forkchoiceUpdated(a2.hash, b2.hash).isLeft shouldBe true + finalizedHash.value shouldBe a1.hash + } + + override protected def startNodes(): Unit = { + ec1.start() + ec1.logPorts() + } + + override protected def stopNodes(): Unit = ec1.stop() + + override protected def setupChain(): Unit = {} + + override protected def step(text: String): Unit = log.debug(text) // No Waves node to print to +} diff --git a/consensus-client-it/src/test/scala/units/block/validation/BaseBlockValidationSuite.scala b/consensus-client-it/src/test/scala/units/block/validation/BaseBlockValidationSuite.scala index 4e6d15f4..36c0d578 100644 --- a/consensus-client-it/src/test/scala/units/block/validation/BaseBlockValidationSuite.scala +++ b/consensus-client-it/src/test/scala/units/block/validation/BaseBlockValidationSuite.scala @@ -221,7 +221,7 @@ trait BaseBlockValidationSuite extends BaseDockerTestSuite { } step(s"Wait actingMiner epoch") - chainContract.waitForMinerEpoch(actingMiner) + chainContract.waitForUnusedMinerEpoch(actingMiner) } protected def setupForNativeTokenTransfer(): Unit = { diff --git a/consensus-client-it/src/test/scala/units/client/HttpChainContractClient.scala b/consensus-client-it/src/test/scala/units/client/HttpChainContractClient.scala index 92db866c..f640a58d 100644 --- a/consensus-client-it/src/test/scala/units/client/HttpChainContractClient.scala +++ b/consensus-client-it/src/test/scala/units/client/HttpChainContractClient.scala @@ -63,6 +63,23 @@ class HttpChainContractClient(api: NodeHttpApi, override val contract: Address) } } + /** Waits for an epoch in which `minerAccount` is the designated miner and no block has been registered yet. Unlike [[waitForMinerEpoch]], it + * doesn't return in an epoch that changed hands midway, e.g. after the previous miner left having already mined in it: the epoch's blocks then + * belong to the previous miner, and `minerAccount` can only mine in a later epoch. + */ + def waitForUnusedMinerEpoch(minerAccount: KeyPair)(implicit loggingOptions: LoggingOptions = LoggingOptions(logRequest = false)): Unit = { + val expectedGenerator = minerAccount.toAddress + if (loggingOptions.logCall) log.debug(s"${loggingOptions.prefix} waitForUnusedMinerEpoch($expectedGenerator)") + + val subsequentLoggingOptions = loggingOptions.copy(logCall = false) + eventually(timeout(fiveBlocks * 2)) { + val epoch = api.height()(using subsequentLoggingOptions) + computedGenerator()(using subsequentLoggingOptions) shouldBe expectedGenerator + withClue(s"Blocks registered in epoch $epoch: ")(getEpochMeta(epoch) shouldBe empty) + withClue("Epoch changed during the check: ")(api.height()(using subsequentLoggingOptions) shouldBe epoch) + } + } + // This is different from blockchain.height, because we wait this from a contract block def waitForEpoch(atLeast: Height, chainId: Long = DefaultMainChainId): Unit = { log.debug(s"waitForEpoch($atLeast)") diff --git a/local-network/deploy/deploy.yml b/local-network/deploy/deploy.yml index 0c40649d..c5977d95 100644 --- a/local-network/deploy/deploy.yml +++ b/local-network/deploy/deploy.yml @@ -2,7 +2,6 @@ services: deploy: build: context: . - network: host pull_policy: never restart: no # Manual start platform: linux/amd64 # HACK for Apple Silicon diff --git a/src/main/scala/units/ELUpdater.scala b/src/main/scala/units/ELUpdater.scala index e71638db..a7718df2 100644 --- a/src/main/scala/units/ELUpdater.scala +++ b/src/main/scala/units/ELUpdater.scala @@ -247,7 +247,7 @@ class ELUpdater( engineApiClient .forkchoiceUpdatedWithPayload( parentBlock.hash, - if (finalizedBlock.height > parentBlock.height) parentBlock.hash else finalizedBlock.hash, + finalizedHashFor(parentBlock, finalizedBlock), nextBlockUnixTs, epochInfo.rewardAddress, prevRandao, @@ -1733,10 +1733,8 @@ class ELUpdater( loop(curState.lastContractBlock, List.empty) } - private def confirmBlock(block: L2BlockLike, finalizedBlock: L2BlockLike): Result[PayloadStatus] = { - val finalizedBlockHash = if (finalizedBlock.height > block.height) block.hash else finalizedBlock.hash - engineApiClient.forkchoiceUpdated(block.hash, finalizedBlockHash) - } + private def confirmBlock(block: L2BlockLike, finalizedBlock: L2BlockLike): Result[PayloadStatus] = + engineApiClient.forkchoiceUpdated(block.hash, finalizedHashFor(block, finalizedBlock)) private def canSupportAnotherAltChain(nodeChainInfo: ChainInfo): Boolean = { val chainSupporters = chainContractClient.getSupporters(nodeChainInfo.id) @@ -1773,6 +1771,19 @@ object ELUpdater { case class EpochInfo(number: Height, miner: Address, rewardAddress: EthAddress, hitSource: ByteStr, prevEpochLastBlockHash: Option[BlockHash]) + /** "No finalized block" in forkchoiceUpdated: EL keeps its previous finalized block. */ + val NoFinalizedBlockHash: BlockHash = BlockHash(EthereumConstants.EmptyBlockHashHex) + + /** The finalized block to report to EL along with the new head `head`. + * + * EL rejects a finalized block above the head, and freezes everything up to the finalized block, assuming it can never be reorged. When `head` is + * below the contract's finalized block, it may not be on the finalized chain (e.g. a block received from a peer and never registered on the + * contract), so it must not be reported as finalized: EL would freeze it, and a later switch to the finalized chain could not replace it. No + * finalized block is reported instead. + */ + def finalizedHashFor(head: L2BlockLike, finalizedBlock: L2BlockLike): BlockHash = + if (finalizedBlock.height > head.height) NoFinalizedBlockHash else finalizedBlock.hash + sealed trait State object State { case object Starting extends State diff --git a/src/main/scala/units/client/contract/ChainContractClient.scala b/src/main/scala/units/client/contract/ChainContractClient.scala index 20f01a4a..18c1216a 100644 --- a/src/main/scala/units/client/contract/ChainContractClient.scala +++ b/src/main/scala/units/client/contract/ChainContractClient.scala @@ -24,7 +24,7 @@ trait ChainContractClient { def extractData(key: String): Option[DataEntry[?]] - def isContractSetup: Boolean = getLongData("minerReward").isDefined + def isContractSetup: Boolean = extractData("minerReward").isDefined def isStopped: Boolean = getBooleanData("stopped").getOrElse(false) diff --git a/src/test/scala/units/FinalizedHashForTestSuite.scala b/src/test/scala/units/FinalizedHashForTestSuite.scala new file mode 100644 index 00000000..8341a141 --- /dev/null +++ b/src/test/scala/units/FinalizedHashForTestSuite.scala @@ -0,0 +1,41 @@ +package units + +import org.scalatest.freespec.AnyFreeSpec +import org.scalatest.matchers.should.Matchers +import units.ELUpdater.{NoFinalizedBlockHash, finalizedHashFor} +import units.client.L2BlockLike +import units.eth.{EthAddress, EthereumConstants} + +class FinalizedHashForTestSuite extends AnyFreeSpec with Matchers { + private def block(blockHeight: Long, n: Int): L2BlockLike = { + val h = BlockHash(f"0x$n%064x") + new L2BlockLike { + override val hash: BlockHash = h + override val parentHash: BlockHash = BlockHash(f"0x${n + 1000}%064x") + override val height: Long = blockHeight + override val minerRewardL2Address: EthAddress = EthAddress.empty + } + } + + private val finalized = block(10, 1) + + "finalizedHashFor" - { + "reports the contract's finalized block when the head is above it" in { + finalizedHashFor(block(11, 2), finalized) shouldBe finalized.hash + } + + "reports the contract's finalized block when it is the head" in { + finalizedHashFor(finalized, finalized) shouldBe finalized.hash + } + + "reports no finalized block when the head is below the contract's finalized block" in { + // The head may be a block that never made it to the contract. Reporting it as finalized makes EL freeze it for good. + finalizedHashFor(block(9, 3), finalized) shouldBe NoFinalizedBlockHash + } + } + + "NoFinalizedBlockHash is the all-zero hash" in { + NoFinalizedBlockHash.str shouldBe EthereumConstants.EmptyBlockHashHex + NoFinalizedBlockHash.str shouldBe "0x" + "0" * 64 + } +} diff --git a/src/test/scala/units/client/TestEcClients.scala b/src/test/scala/units/client/TestEcClients.scala index 272a53bf..09d13838 100644 --- a/src/test/scala/units/client/TestEcClients.scala +++ b/src/test/scala/units/client/TestEcClients.scala @@ -10,7 +10,7 @@ import units.client.engine.EngineApiClient.PayloadId import units.client.engine.model.* import units.client.engine.{EngineApiClient, LoggedEngineApiClient} import units.eth.{EthAddress, EthereumConstants} -import units.{BlockHash, Result, NetworkL2Block} +import units.{BlockHash, ELUpdater, NetworkL2Block, Result} import units.test.take /** Life of a block: @@ -144,8 +144,10 @@ class TestEcClients private ( } log.debug(s"Chain: ${updatedChain.map(_.hash.take(LogChars)).mkString(" -> ")}") - PayloadStatus.Valid - }.asRight + // Like geth: an all-zero finalized hash means "no finalized block", otherwise it must be on the chain of the new head + if (finalizedBlockHash == ELUpdater.NoFinalizedBlockHash || updatedChain.exists(_.hash == finalizedBlockHash)) PayloadStatus.Valid.asRight + else s"Payload validation error: final block $finalizedBlockHash not in canonical chain".asLeft + } override def forkchoiceUpdatedWithPayload( lastBlockHash: BlockHash, @@ -158,8 +160,10 @@ class TestEcClients private ( requestId: Int ): Result[PayloadId] = { forkChoiceUpdateWithPayloadIdCalls.increment() - forkchoiceUpdated(lastBlockHash, finalizedBlockHash, requestId) + forkchoiceUpdated(lastBlockHash, finalizedBlockHash, requestId).flatMap(_ => buildPayload(lastBlockHash)) + } + private def buildPayload(lastBlockHash: BlockHash): Result[PayloadId] = { val nextFutureBlock = futureBlocks.transformAndExtract { xs => val (found, rest) = xs.partition(_.parentHash == lastBlockHash) found match {