fix(coordinator): make runtime invariants explicit - #3716
Conversation
Replace unsafe proof and gas-cap dereferences with contextual invariant checks, and cover missing prover responses with regression tests. Refs #3686 Signed-off-by: nadeemb53 <nadeemb53@gmail.com>
There was a problem hiding this comment.
Pull request overview
This PR makes several coordinator runtime invariants explicit to avoid bare NullPointerExceptions and to improve diagnosability when prover or blob compression proof data is missing, with regression tests to lock in the new behavior.
Changes:
- Replace non-null assertions with named
requireNotNull(...)invariants and actionable error messages (gas caps, blob compression proofs, and prover responses). - Validate blob compression proofs at smart-contract submission/finalization boundaries.
- Add regression tests ensuring missing prover responses surface the exact missing
proofIndex.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| coordinator/ethereum/gas-pricing/dynamic-cap/src/main/kotlin/net/consensys/linea/ethereum/gaspricing/dynamiccap/GasPriceCapProviderImplV2.kt | Replaces !! with requireNotNull for maxBaseFeePerGasCap before applying coefficient. |
| coordinator/clients/smart-contract-client/src/main/kotlin/net/consensys/linea/contract/l1/Web3JLinethRollupSmartContractClient.kt | Adds a boundary check (requireCompressionProofs) to fail fast with blob index when a compression proof is missing. |
| coordinator/clients/smart-contract-client/src/main/kotlin/net/consensys/linea/contract/l1/Web3JLinethRollupFunctionBuilders.kt | Makes finalization builder invariant explicit by extracting compressionProof via requireNotNull. |
| coordinator/clients/smart-contract-client/src/main/kotlin/net/consensys/linea/contract/l1/Web3JLineaValidiumFunctionBuilders.kt | Same finalization invariant improvement for validium finalization builder. |
| coordinator/clients/smart-contract-client/src/main/kotlin/net/consensys/linea/contract/l1/FunctionBuildersV8.kt | Same finalization invariant improvement for V8 finalization function builder. |
| coordinator/clients/prover-client/riscv-client/src/main/kotlin/linea/coordinator/clients/prover/riscv/RollupProverClient.kt | Reports the exact proofIndex when an L2 execution proof response is missing (null response). |
| coordinator/clients/prover-client/riscv-client/src/main/kotlin/linea/coordinator/clients/prover/riscv/RollupAggregationProverClient.kt | Reports the exact proofIndex when a rollup proof response is missing (null response). |
| coordinator/clients/prover-client/riscv-client/src/test/kotlin/linea/coordinator/clients/prover/riscv/RiscVProofRequestDtoMapperTest.kt | Adds regression coverage asserting the new missing-response error messages and exception types. |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
Signed-off-by: nadeemb53 <nadeemb53@gmail.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 12 out of 12 changed files in this pull request and generated no new comments.
Suppressed comments (3)
coordinator/clients/smart-contract-client/src/test/kotlin/net/consensys/linea/contract/l1/Web3JLinethRollupSmartContractClientTest.kt:53
- Same spy-stubbing issue as above:
whenever(client.getVersion())will call the realgetVersion()during stubbing. UsedoReturn(...).whenever(client).getVersion()to ensure the test doesn't depend on the real implementation.
fun `rejects blob submission without a compression proof`() {
whenever(client.getVersion()).thenReturn(SafeFuture.completedFuture(LinethRollupContractVersion.V6))
val blob = createBlobRecord(startBlockNumber = 1UL, endBlockNumber = 2UL)
.copy(blobCompressionProof = null)
coordinator/ethereum/gas-pricing/dynamic-cap/src/test/kotlin/net/consensys/linea/ethereum/gaspricing/dynamiccap/GasPriceCapProviderImplV2Test.kt:185
whenever(spy.getGasPriceCaps(...))on a Mockito spy will invoke the realgetGasPriceCapsduring stubbing, which can make the test flaky (and can fail if the real method throws/returns null). UsedoReturn(...).whenever(spy).getGasPriceCaps(...)to stub without calling the real method.
val gasPriceCapProvider = spy(createGasPriceCapProvider())
whenever(gasPriceCapProvider.getGasPriceCaps(targetBlockTime)).thenReturn(
tech.pegasys.teku.infrastructure.async.SafeFuture.completedFuture(
coordinator/clients/smart-contract-client/src/test/kotlin/net/consensys/linea/contract/l1/Web3JLinethRollupSmartContractClientTest.kt:43
whenever(client.getVersion())is stubbing a method on a Mockito spy, which calls the realgetVersion()during stubbing. If the real implementation touches Web3j/contract wrappers, this can introduce nondeterminism. PreferdoReturn(...).whenever(client).getVersion()to avoid invoking the real method.
This issue also appears on line 50 of the same file.
fun `submits compression-proven blobs`() {
whenever(client.getVersion()).thenReturn(SafeFuture.completedFuture(LinethRollupContractVersion.V6))
val blob = createBlobRecord(startBlockNumber = 1UL, endBlockNumber = 2UL).let {
it.copy(blobCompressionProof = it.blobCompressionProof!!.copy(expectedY = ByteArray(32) { 1 }))
Signed-off-by: nadeemb53 <nadeemb53@gmail.com>
Signed-off-by: nadeemb53 <nadeemb53@gmail.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 14 out of 14 changed files in this pull request and generated no new comments.
Suppressed comments (2)
coordinator/clients/smart-contract-client/src/main/kotlin/net/consensys/linea/contract/l1/Web3JLinethRollupSmartContractClient.kt:144
- Same as
submitBlobs:requireCompressionProofs()currently runs only aftergetVersion()completes, which can perform a network fetch on cache misses. You can validate first (still asynchronously) to fail fast without doing an unnecessary RPC.
override fun submitBlobsEthCall(blobs: List<BlobRecord>, gasPriceCaps: GasPriceCaps?): SafeFuture<String?> {
return getVersion()
.thenCompose { version ->
val compressionProofs = blobs.requireCompressionProofs("submitBlobsEthCall")
val function = Web3JLinethRollupFunctionBuilders.buildSubmitBlobsFunction(version, blobs)
coordinator/clients/smart-contract-client/src/main/kotlin/net/consensys/linea/contract/l1/Web3JLinethRollupSmartContractClient.kt:131
requireCompressionProofs()is evaluated only aftergetVersion()completes. SincegetVersion()may hit the network (on cache misses), invalid input can still trigger an unnecessary RPC. Consider validating the blob compression proofs before callinggetVersion()while still returning a failedSafeFuture(to avoid throwing synchronously).
This issue also appears on line 140 of the same file.
override fun submitBlobs(blobs: List<BlobRecord>, gasPriceCaps: GasPriceCaps?): SafeFuture<String> {
return getVersion()
.thenCompose { version ->
val compressionProofs = blobs.requireCompressionProofs("submitBlobs")
val function = Web3JLinethRollupFunctionBuilders.buildSubmitBlobsFunction(version, blobs)
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 14 out of 14 changed files in this pull request and generated no new comments.
Suppressed comments (2)
coordinator/clients/prover-client/riscv-client/src/testFixtures/kotlin/linea/coordinator/clients/prover/riscv/FakeProverProofTransports.kt:81
- Same issue as above:
awaitResponsethrows synchronously whenresponseProviderreturns null. It should return aSafeFuturethat completes exceptionally so call sites behave like the real transports.
override fun awaitResponse(proofIndex: BlockIntervalProofIndex): SafeFuture<RollupProofResponseDto> =
SafeFuture.completedFuture(requireNotNull(response(proofIndex)))
coordinator/clients/prover-client/riscv-client/src/testFixtures/kotlin/linea/coordinator/clients/prover/riscv/FakeProverProofTransports.kt:36
awaitResponsecurrently evaluatesrequireNotNull(response(proofIndex))before creating theSafeFuture, so when the response is missing this fake transport throws synchronously instead of returning a failedSafeFuture. That breaks theProverProofTransport.awaitResponsecontract and can change failure behavior in tests that expect async failures.
This issue also appears on line 80 of the same file.
override fun awaitResponse(proofIndex: BlockIntervalProofIndex): SafeFuture<L2ExecutionProofResponseDto> =
SafeFuture.completedFuture(requireNotNull(response(proofIndex)))
Signed-off-by: nadeemb53 <nadeemb53@gmail.com>
Signed-off-by: nadeemb53 <nadeemb53@gmail.com>
| blobs = blobs.map { it.blobCompressionProof!!.compressedData }, | ||
| gasPriceCaps = gasPriceCaps, | ||
| ) | ||
| return SafeFuture.completedFuture(Unit) |
There was a problem hiding this comment.
IMO this makes the code unnecessarily more complex (and slightly inefficient) just to avoid a !!.
We shall be more critical of taking AI feedback.
In this context, we cannot have nulls.
- The submission controller shall only fetch proven records
- integration/e2e tests will catch this, so it won't make it to prod.
- This change does not add extra safety/soundness; it just adds a better error message if it happens...
Refs #3686