feat: moving fee payout + make proof submission sequential (#8262)

Fixing #7622 and #8259

Author: LHerskind

l1-contracts/src/core/Rollup.sol

@@ -39,7 +39,6 @@ contract Rollup is Leonidas, IRollup, ITestRollup {
     bytes32 archive;
     bytes32 blockHash;
     uint128 slotNumber;
-    bool isProven;
   }
 
   // @note  The number of slots within which a block must be proven
@@ -97,8 +96,7 @@ contract Rollup is Leonidas, IRollup, ITestRollup {
     blocks[0] = BlockLog({
       archive: bytes32(Constants.GENESIS_ARCHIVE_ROOT),
       blockHash: bytes32(0),
-      slotNumber: 0,
-      isProven: true
+      slotNumber: 0
     });
     pendingBlockCount = 1;
     provenBlockCount = 1;
@@ -151,11 +149,7 @@ contract Rollup is Leonidas, IRollup, ITestRollup {
     onlyOwner
   {
     if (blockNumber > provenBlockCount && blockNumber <= pendingBlockCount) {
-      for (uint256 i = provenBlockCount; i < blockNumber; i++) {
-        blocks[i].isProven = true;
-        emit L2ProofVerified(i, "CHEAT");
-      }
-      _progressState();
+      provenBlockCount = blockNumber;
     }
     assumeProvenUntilBlockNumber = blockNumber;
   }
@@ -274,6 +268,12 @@ contract Rollup is Leonidas, IRollup, ITestRollup {
       revert Errors.Rollup__TryingToProveNonExistingBlock();
     }
 
+    // @note  This implicitly also ensures that we have not already proven, since
+    //        the value `provenBlockCount` is incremented at the end of this function
+    if (header.globalVariables.blockNumber != provenBlockCount) {
+      revert Errors.Rollup__NonSequentialProving();
+    }
+
     bytes32 expectedLastArchive = blocks[header.globalVariables.blockNumber - 1].archive;
     // We do it this way to provide better error messages than passing along the storage values
     // TODO(#4148) Proper genesis state. If the state is empty, we allow anything for now.
@@ -356,24 +356,22 @@ contract Rollup is Leonidas, IRollup, ITestRollup {
       revert Errors.Rollup__InvalidProof();
     }
 
-    blocks[header.globalVariables.blockNumber].isProven = true;
+    provenBlockCount += 1;
 
-    _progressState();
+    for (uint256 i = 0; i < 32; i++) {
+      address coinbase = address(uint160(uint256(publicInputs[25 + i * 2])));
+      uint256 fees = uint256(publicInputs[26 + i * 2]);
 
+      if (coinbase != address(0) && fees > 0) {
+        // @note  This will currently fail if there are insufficient funds in the bridge
+        //        which WILL happen for the old version after an upgrade where the bridge follow.
+        //        Consider allowing a failure. See #7938.
+        FEE_JUICE_PORTAL.distributeFees(coinbase, fees);
+      }
+    }
     emit L2ProofVerified(header.globalVariables.blockNumber, _proverId);
   }
 
-  /**
-   * @notice  Get the `isProven` flag for the block number
-   *
-   * @param _blockNumber - The block number to check
-   *
-   * @return bool - True if proven, false otherwise
-   */
-  function isBlockProven(uint256 _blockNumber) external view override(IRollup) returns (bool) {
-    return blocks[_blockNumber].isProven;
-  }
-
   /**
    * @notice  Get the archive root of a specific block
    *
@@ -476,8 +474,7 @@ contract Rollup is Leonidas, IRollup, ITestRollup {
     blocks[pendingBlockCount++] = BlockLog({
       archive: _archive,
       blockHash: _blockHash,
-      slotNumber: header.globalVariables.slotNumber.toUint128(),
-      isProven: false
+      slotNumber: header.globalVariables.slotNumber.toUint128()
     });
 
     // @note  The block number here will always be >=1 as the genesis block is at 0
@@ -494,22 +491,20 @@ contract Rollup is Leonidas, IRollup, ITestRollup {
       header.globalVariables.blockNumber, header.contentCommitment.outHash, l2ToL1TreeMinHeight
     );
 
-    // @note  This should be addressed at the time of proving if sequential proving or at the time of
-    //        inclusion into the proven chain otherwise. See #7622.
-    if (header.globalVariables.coinbase != address(0) && header.totalFees > 0) {
-      // @note  This will currently fail if there are insufficient funds in the bridge
-      //        which WILL happen for the old version after an upgrade where the bridge follow.
-      //        Consider allowing a failure. See #7938.
-      FEE_JUICE_PORTAL.distributeFees(header.globalVariables.coinbase, header.totalFees);
-    }
-
     emit L2BlockProcessed(header.globalVariables.blockNumber);
 
     // Automatically flag the block as proven if we have cheated and set assumeProvenUntilBlockNumber.
     if (header.globalVariables.blockNumber < assumeProvenUntilBlockNumber) {
-      blocks[header.globalVariables.blockNumber].isProven = true;
+      provenBlockCount += 1;
+
+      if (header.globalVariables.coinbase != address(0) && header.totalFees > 0) {
+        // @note  This will currently fail if there are insufficient funds in the bridge
+        //        which WILL happen for the old version after an upgrade where the bridge follow.
+        //        Consider allowing a failure. See #7938.
+        FEE_JUICE_PORTAL.distributeFees(header.globalVariables.coinbase, header.totalFees);
+      }
+
       emit L2ProofVerified(header.globalVariables.blockNumber, "CHEAT");
-      _progressState();
     }
   }
 
@@ -537,37 +532,6 @@ contract Rollup is Leonidas, IRollup, ITestRollup {
     return blocks[pendingBlockCount - 1].archive;
   }
 
-  /**
-   * @notice  Progresses the state of the proven chain as far as possible
-   *
-   * @dev     Emits `ProgressedState` if the state is progressed
-   *
-   * @dev     Will continue along the pending chain as long as the blocks are proven
-   *          stops at the first unproven block.
-   *
-   * @dev     Have a potentially unbounded gas usage. @todo Will need a bounded version, such that it cannot be
-   *          used as a DOS vector.
-   */
-  function _progressState() internal {
-    if (pendingBlockCount == provenBlockCount) {
-      // We are already up to date
-      return;
-    }
-
-    uint256 cachedProvenBlockCount = provenBlockCount;
-
-    for (; cachedProvenBlockCount < pendingBlockCount; cachedProvenBlockCount++) {
-      if (!blocks[cachedProvenBlockCount].isProven) {
-        break;
-      }
-    }
-
-    if (cachedProvenBlockCount > provenBlockCount) {
-      provenBlockCount = cachedProvenBlockCount;
-      emit ProgressedState(provenBlockCount, pendingBlockCount);
-    }
-  }
-
   /**
    * @notice  Validates the header for submission
    *

l1-contracts/src/core/interfaces/IRollup.sol

@@ -18,7 +18,6 @@ interface ITestRollup {
 interface IRollup {
   event L2BlockProcessed(uint256 indexed blockNumber);
   event L2ProofVerified(uint256 indexed blockNumber, bytes32 indexed proverId);
-  event ProgressedState(uint256 provenBlockCount, uint256 pendingBlockCount);
   event PrunedPending(uint256 provenBlockCount, uint256 pendingBlockCount);
 
   function canProposeAtTime(uint256 _ts, address _proposer, bytes32 _archive)
@@ -81,6 +80,5 @@ interface IRollup {
   // ) external;
 
   function archive() external view returns (bytes32);
-  function isBlockProven(uint256 _blockNumber) external view returns (bool);
   function archiveAt(uint256 _blockNumber) external view returns (bytes32);
 }

l1-contracts/src/core/libraries/Errors.sol

@@ -61,6 +61,7 @@ library Errors {
   error Rollup__UnavailableTxs(bytes32 txsHash); // 0x414906c3
   error Rollup__NothingToPrune(); // 0x850defd3
   error Rollup__NotReadyToPrune(uint256 currentSlot, uint256 prunableAt); // 0x9fdf1614
+  error Rollup__NonSequentialProving(); // 0x1e5be132
 
   // Registry
   error Registry__RollupNotRegistered(address rollup); // 0xa1fee4cf

l1-contracts/test/Rollup.t.sol

@@ -78,6 +78,27 @@ contract RollupTest is DecoderBase {
     _;
   }
 
+  function testRevertProveTwice() public setUpFor("mixed_block_1") {
+    DecoderBase.Data memory data = load("mixed_block_1").block;
+    bytes memory header = data.header;
+    bytes32 archive = data.archive;
+    bytes memory body = data.body;
+
+    // Progress time as necessary
+    vm.warp(max(block.timestamp, data.decodedHeader.globalVariables.timestamp));
+    availabilityOracle.publish(body);
+
+    // We jump to the time of the block. (unless it is in the past)
+    vm.warp(max(block.timestamp, data.decodedHeader.globalVariables.timestamp));
+
+    rollup.process(header, archive, bytes32(0));
+
+    rollup.submitBlockRootProof(header, archive, bytes32(0), "", "");
+
+    vm.expectRevert(abi.encodeWithSelector(Errors.Rollup__NonSequentialProving.selector));
+    rollup.submitBlockRootProof(header, archive, bytes32(0), "", "");
+  }
+
   function testTimestamp() public setUpFor("mixed_block_1") {
     // Ensure that the timestamp of the current slot is never in the future.
     for (uint256 i = 0; i < 100; i++) {
@@ -105,7 +126,7 @@ contract RollupTest is DecoderBase {
     _testBlock("mixed_block_1", false);
 
     uint256 currentSlot = rollup.getCurrentSlot();
-    (,, uint128 slot,) = rollup.blocks(1);
+    (,, uint128 slot) = rollup.blocks(1);
     uint256 prunableAt = uint256(slot) + rollup.TIMELINESS_PROVING_IN_SLOTS();
 
     vm.expectRevert(
@@ -127,7 +148,7 @@ contract RollupTest is DecoderBase {
     //        Even if we end up reverting block 1, we should still see the same root in the inbox.
     bytes32 inboxRoot2 = inbox.getRoot(2);
 
-    (,, uint128 slot,) = rollup.blocks(1);
+    (,, uint128 slot) = rollup.blocks(1);
     uint256 prunableAt = uint256(slot) + rollup.TIMELINESS_PROVING_IN_SLOTS();
 
     uint256 timeOfPrune = rollup.getTimestampForSlot(prunableAt);
@@ -198,6 +219,14 @@ contract RollupTest is DecoderBase {
     // We jump to the time of the block. (unless it is in the past)
     vm.warp(max(block.timestamp, data.decodedHeader.globalVariables.timestamp));
 
+    address coinbase = data.decodedHeader.globalVariables.coinbase;
+    uint256 coinbaseBalance = portalERC20.balanceOf(coinbase);
+    assertEq(coinbaseBalance, 0, "invalid initial coinbase balance");
+
+    // Assert that balance have NOT been increased by proposing the block
+    rollup.process(header, archive, bytes32(0));
+    assertEq(portalERC20.balanceOf(coinbase), 0, "invalid coinbase balance");
+
     vm.expectRevert(
       abi.encodeWithSelector(
         IERC20Errors.ERC20InsufficientBalance.selector,
@@ -206,15 +235,13 @@ contract RollupTest is DecoderBase {
         feeAmount
       )
     );
-    rollup.process(header, archive, bytes32(0));
-
-    address coinbase = data.decodedHeader.globalVariables.coinbase;
-    uint256 coinbaseBalance = portalERC20.balanceOf(coinbase);
-    assertEq(coinbaseBalance, 0, "invalid initial coinbase balance");
+    rollup.submitBlockRootProof(header, archive, bytes32(0), "", "");
+    assertEq(portalERC20.balanceOf(coinbase), 0, "invalid coinbase balance");
 
     portalERC20.mint(address(feeJuicePortal), feeAmount - portalBalance);
 
-    rollup.process(header, archive, bytes32(0));
+    // When the block is proven we should have received the funds
+    rollup.submitBlockRootProof(header, archive, bytes32(0), "", "");
     assertEq(portalERC20.balanceOf(coinbase), feeAmount, "invalid coinbase balance");
   }
 
@@ -237,9 +264,18 @@ contract RollupTest is DecoderBase {
 
   function testConsecutiveMixedBlocksNonSequentialProof() public setUpFor("mixed_block_1") {
     _testBlock("mixed_block_1", false);
-    _testBlock("mixed_block_2", true);
 
-    assertTrue(rollup.isBlockProven(2), "Block 2 is not proven");
+    DecoderBase.Data memory data = load("mixed_block_2").block;
+    bytes memory header = data.header;
+    bytes32 archive = data.archive;
+    bytes memory body = data.body;
+
+    vm.warp(max(block.timestamp, data.decodedHeader.globalVariables.timestamp));
+    availabilityOracle.publish(body);
+    rollup.process(header, archive, bytes32(0));
+
+    vm.expectRevert(abi.encodeWithSelector(Errors.Rollup__NonSequentialProving.selector));
+    rollup.submitBlockRootProof(header, archive, bytes32(0), "", "");
 
     assertEq(rollup.pendingBlockCount(), 3, "Invalid pending block count");
     assertEq(rollup.provenBlockCount(), 1, "Invalid proven block count");
@@ -366,9 +402,8 @@ contract RollupTest is DecoderBase {
 
   function testSubmitProofInvalidArchive() public setUpFor("empty_block_1") {
     _testBlock("empty_block_1", false);
-    _testBlock("empty_block_2", false);
 
-    DecoderBase.Data memory data = load("empty_block_2").block;
+    DecoderBase.Data memory data = load("empty_block_1").block;
     bytes memory header = data.header;
     bytes32 archive = data.archive;
 
@@ -379,7 +414,7 @@ contract RollupTest is DecoderBase {
 
     vm.expectRevert(
       abi.encodeWithSelector(
-        Errors.Rollup__InvalidArchive.selector, rollup.archiveAt(1), 0xdeadbeef
+        Errors.Rollup__InvalidArchive.selector, rollup.archiveAt(0), 0xdeadbeef
       )
     );
     rollup.submitBlockRootProof(header, archive, bytes32(0), "", "");
@@ -433,12 +468,11 @@ contract RollupTest is DecoderBase {
     rollup.process(header, archive, bytes32(0));
 
     if (_submitProof) {
+      uint256 pre = rollup.provenBlockCount();
+
       rollup.submitBlockRootProof(header, archive, bytes32(0), "", "");
 
-      assertTrue(
-        rollup.isBlockProven(full.block.decodedHeader.globalVariables.blockNumber),
-        "Block not proven"
-      );
+      assertEq(pre + 1, rollup.provenBlockCount(), "Block not proven");
     }
 
     bytes32 l2ToL1MessageTreeRoot;

yarn-project/end-to-end/src/e2e_fees/failures.test.ts

@@ -8,6 +8,7 @@ import {
   PublicFeePaymentMethod,
   TxStatus,
   computeSecretHash,
+  sleep,
 } from '@aztec/aztec.js';
 import { Gas, GasSettings } from '@aztec/circuits.js';
 import { FunctionType } from '@aztec/foundation/abi';
@@ -94,9 +95,16 @@ describe('e2e_fees failures', () => {
       .wait({ dontThrowOnRevert: true });
 
     expect(txReceipt.status).toBe(TxStatus.APP_LOGIC_REVERTED);
+
+    // We wait until the block is proven since that is when the payout happens.
+    const bn = await t.aztecNode.getBlockNumber();
+    while ((await t.aztecNode.getProvenBlockNumber()) < bn) {
+      await sleep(1000);
+    }
+
     const feeAmount = txReceipt.transactionFee!;
-    const newSequencerL1Gas = await t.getCoinbaseBalance();
-    expect(newSequencerL1Gas).toEqual(currentSequencerL1Gas + feeAmount);
+    const newSequencerL1FeeAssetBalance = await t.getCoinbaseBalance();
+    expect(newSequencerL1FeeAssetBalance).toEqual(currentSequencerL1Gas + feeAmount);
 
     // and thus we paid the fee
     await expectMapping(

yarn-project/end-to-end/src/e2e_fees/fees_test.ts

@@ -27,6 +27,7 @@ import {
   TokenContract,
 } from '@aztec/noir-contracts.js';
 import { getCanonicalFeeJuice } from '@aztec/protocol-contracts/fee-juice';
+import { type ProverNode } from '@aztec/prover-node';
 
 import { getContract } from 'viem';
 
@@ -53,6 +54,7 @@ export class FeesTest {
   public logger: DebugLogger;
   public pxe!: PXE;
   public aztecNode!: AztecNode;
+  public proverNode!: ProverNode;
 
   public aliceWallet!: AccountWallet;
   public aliceAddress!: AztecAddress;
@@ -167,9 +169,10 @@ export class FeesTest {
     await this.snapshotManager.snapshot(
       'initial_accounts',
       addAccounts(3, this.logger),
-      async ({ accountKeys }, { pxe, aztecNode, aztecNodeConfig }) => {
+      async ({ accountKeys }, { pxe, aztecNode, aztecNodeConfig, proverNode }) => {
         this.pxe = pxe;
         this.aztecNode = aztecNode;
+        this.proverNode = proverNode;
         const accountManagers = accountKeys.map(ak => getSchnorrAccount(pxe, ak[0], ak[1], 1));
         await Promise.all(accountManagers.map(a => a.register()));
         this.wallets = await Promise.all(accountManagers.map(a => a.getWallet()));

yarn-project/end-to-end/src/e2e_fees/private_payments.test.ts

@@ -6,6 +6,7 @@ import {
   PrivateFeePaymentMethod,
   type TxReceipt,
   computeSecretHash,
+  sleep,
 } from '@aztec/aztec.js';
 import { type GasSettings } from '@aztec/circuits.js';
 import { type TokenContract as BananaCoin, FPCContract } from '@aztec/noir-contracts.js';
@@ -138,6 +139,12 @@ describe('e2e_fees private_payment', () => {
      * TODO(6583): update this comment properly now that public execution consumes gas
      */
 
+    // We wait until the block is proven since that is when the payout happens.
+    const bn = await t.aztecNode.getBlockNumber();
+    while ((await t.aztecNode.getProvenBlockNumber()) < bn) {
+      await sleep(1000);
+    }
+
     // expect(tx.transactionFee).toEqual(200032492n);
     await expect(t.getCoinbaseBalance()).resolves.toEqual(InitialSequencerL1Gas + tx.transactionFee!);
     const [feeAmount, refundAmount] = getFeeAndRefund(tx);