Suggestions from code review

Author: spalladino

l1-contracts/src/core/Rollup.sol

@@ -102,7 +102,7 @@ contract Rollup is IStaking, IValidatorSelection, IRollup, RollupCore {
    * @param _blobsHash - The blobs hash for this block
    * @param _flags - The flags to validate
    */
-  function validateHeader(
+  function validateHeaderWithAttestations(
     ProposedHeader calldata _header,
     CommitteeAttestations memory _attestations,
     address[] calldata _signers,
@@ -111,7 +111,7 @@ contract Rollup is IStaking, IValidatorSelection, IRollup, RollupCore {
     BlockHeaderValidationFlags memory _flags
   ) external override(IRollup) {
     Timestamp currentTime = Timestamp.wrap(block.timestamp);
-    ExtRollupLib.validateHeader(
+    ExtRollupLib.validateHeaderWithAttestations(
       ValidateHeaderArgs({
         header: _header,
         digest: _digest,

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

@@ -146,7 +146,7 @@ interface IRollupCore {
 }
 
 interface IRollup is IRollupCore, IHaveVersion {
-  function validateHeader(
+  function validateHeaderWithAttestations(
     ProposedHeader calldata _header,
     CommitteeAttestations memory _attestations,
     address[] memory _signers,

l1-contracts/src/core/libraries/rollup/EpochProofLib.sol

@@ -8,7 +8,7 @@ import {
   IRollupCore,
   RollupStore
 } from "@aztec/core/interfaces/IRollup.sol";
-import {TempBlockLog} from "@aztec/core/libraries/compressed-data/BlockLog.sol";
+import {CompressedTempBlockLog} from "@aztec/core/libraries/compressed-data/BlockLog.sol";
 import {ChainTipsLib, CompressedChainTips} from "@aztec/core/libraries/compressed-data/Tips.sol";
 import {Constants} from "@aztec/core/libraries/ConstantsGen.sol";
 import {Errors} from "@aztec/core/libraries/Errors.sol";
@@ -18,6 +18,7 @@ import {RewardLib} from "@aztec/core/libraries/rollup/RewardLib.sol";
 import {STFLib} from "@aztec/core/libraries/rollup/STFLib.sol";
 import {ValidatorSelectionLib} from "@aztec/core/libraries/rollup/ValidatorSelectionLib.sol";
 import {Timestamp, Slot, Epoch, TimeLib} from "@aztec/core/libraries/TimeLib.sol";
+import {CompressedSlot, CompressedTimeMath} from "@aztec/shared/libraries/CompressedTimeMath.sol";
 import {CommitteeAttestations, SignatureLib} from "@aztec/shared/libraries/SignatureLib.sol";
 import {Math} from "@oz/utils/math/Math.sol";
 import {SafeCast} from "@oz/utils/math/SafeCast.sol";
@@ -30,6 +31,7 @@ library EpochProofLib {
   using SafeCast for uint256;
   using ChainTipsLib for CompressedChainTips;
   using SignatureLib for CommitteeAttestations;
+  using CompressedTimeMath for CompressedSlot;
 
   // This is a temporary struct to avoid stack too deep errors
   struct BlobVarsTemp {
@@ -224,7 +226,7 @@ library EpochProofLib {
     CommitteeAttestations memory _attestations
   ) private {
     // Get the stored attestation hash and payload digest for the last block
-    TempBlockLog memory blockLog = STFLib.getTempBlockLog(_endBlockNumber);
+    CompressedTempBlockLog storage blockLog = STFLib.getStorageTempBlockLog(_endBlockNumber);
 
     // Verify that the provided attestations match the stored hash
     bytes32 providedAttestationsHash = keccak256(abi.encode(_attestations));
@@ -233,10 +235,10 @@ library EpochProofLib {
     );
 
     // Get the slot and epoch for the last block
-    Slot slot = blockLog.slotNumber;
+    Slot slot = blockLog.slotNumber.decompress();
     Epoch epoch = STFLib.getEpochForBlock(_endBlockNumber);
 
-    ValidatorSelectionLib.verify(slot, epoch, _attestations, blockLog.payloadDigest);
+    ValidatorSelectionLib.verifyAttestations(slot, epoch, _attestations, blockLog.payloadDigest);
   }
 
   function assertAcceptable(uint256 _start, uint256 _end) private view returns (Epoch) {

l1-contracts/src/core/libraries/rollup/ExtRollupLib.sol

@@ -29,7 +29,7 @@ library ExtRollupLib {
     EpochProofLib.submitEpochRootProof(_args);
   }
 
-  function validateHeader(
+  function validateHeaderWithAttestations(
     ValidateHeaderArgs calldata _args,
     CommitteeAttestations calldata _attestations,
     address[] calldata _signers
@@ -41,7 +41,7 @@ library ExtRollupLib {
 
     Slot slot = _args.header.slotNumber;
     Epoch epoch = slot.epochFromSlot();
-    ValidatorSelectionLib.verify(slot, epoch, _attestations, _args.digest);
+    ValidatorSelectionLib.verifyAttestations(slot, epoch, _attestations, _args.digest);
     ValidatorSelectionLib.verifyProposer(slot, epoch, _attestations, _signers, _args.digest);
   }
 

l1-contracts/src/core/libraries/rollup/InvalidateLib.sol

@@ -58,10 +58,6 @@ library InvalidateLib {
       revert Errors.Rollup__AttestationsAreValid();
     }
 
-    // Reset the pending block number to remove this block and all subsequent blocks
-    RollupStore storage rollupStore = STFLib.getStorage();
-    rollupStore.tips = rollupStore.tips.updatePendingBlockNumber(_blockNumber - 1);
-
     _invalidateBlock(_blockNumber);
   }
 

l1-contracts/src/core/libraries/rollup/ValidatorSelectionLib.sol

@@ -40,7 +40,6 @@ library ValidatorSelectionLib {
     uint256 needed;
     uint256 signaturesRecovered;
     address[] reconstructedCommittee;
-    bool proposerVerified;
   }
 
   bytes32 private constant VALIDATOR_SELECTION_STORAGE_POSITION =
@@ -98,32 +97,38 @@ library ValidatorSelectionLib {
     address[] memory _signers,
     bytes32 _digest
   ) internal {
-    // Load the committee commitment for the epoch
-    (bytes32 committeeCommitment, uint256 committeeSize) = getCommitteeCommitmentAt(_epochNumber);
+    // Try load the proposer from cache
+    (address proposer, uint256 proposerIndex) = getCachedProposer(_slot);
 
-    // If the target committee size is 0, we skip the validation
-    if (committeeSize == 0) {
-      return;
-    }
+    // If not in cache, grab from the committee, reconstructed from the attestations and signers
+    if (proposer == address(0)) {
+      // Load the committee commitment for the epoch
+      (bytes32 committeeCommitment, uint256 committeeSize) = getCommitteeCommitmentAt(_epochNumber);
 
-    // Reconstruct the committee from the attestations and signers
-    address[] memory committee =
-      _attestations.reconstructCommitteeFromSigners(_signers, committeeSize);
+      // If the target committee size is 0, we skip the validation
+      if (committeeSize == 0) {
+        return;
+      }
 
-    // Check it matches the expected one
-    bytes32 reconstructedCommitment = computeCommitteeCommitment(committee);
-    if (reconstructedCommitment != committeeCommitment) {
-      revert Errors.ValidatorSelection__InvalidCommitteeCommitment(
-        reconstructedCommitment, committeeCommitment
-      );
-    }
+      // Reconstruct the committee from the attestations and signers
+      address[] memory committee =
+        _attestations.reconstructCommitteeFromSigners(_signers, committeeSize);
 
-    // Get the proposer from the committee based on the epoch, slot, and sample seed
-    uint224 sampleSeed = getSampleSeed(_epochNumber);
-    uint256 proposerIndex = computeProposerIndex(_epochNumber, _slot, sampleSeed, committeeSize);
-    address proposer = committee[proposerIndex];
+      // Check it matches the expected one
+      bytes32 reconstructedCommitment = computeCommitteeCommitment(committee);
+      if (reconstructedCommitment != committeeCommitment) {
+        revert Errors.ValidatorSelection__InvalidCommitteeCommitment(
+          reconstructedCommitment, committeeCommitment
+        );
+      }
 
-    setCachedProposer(_slot, proposer, proposerIndex);
+      // Get the proposer from the committee based on the epoch, slot, and sample seed
+      uint224 sampleSeed = getSampleSeed(_epochNumber);
+      proposerIndex = computeProposerIndex(_epochNumber, _slot, sampleSeed, committeeSize);
+      proposer = committee[proposerIndex];
+
+      setCachedProposer(_slot, proposer, proposerIndex);
+    }
 
     // If the proposer is who sent the tx, we're good
     if (proposer == msg.sender) {
@@ -145,20 +150,18 @@ library ValidatorSelectionLib {
   /**
    * @notice  Propose a pending block from the point-of-view of sequencer selection. Will:
    *          - Setup the epoch if needed (if epoch committee is empty skips the rest)
-   *          - Validate that the proposer has signed with its own key
    *          - Validate that the signatures for attestations are indeed from the validatorset
    *          - Validate that the number of valid attestations is sufficient
    *
    * @dev     Cases where errors are thrown:
    *          - If the epoch is not setup
-   *          - If the proposer is not the real proposer AND the proposer is not open
    *          - If the number of valid attestations is insufficient
    *
    * @param _slot - The slot of the block
    * @param _attestations - The signatures (or empty; just address is provided) of the committee members
    * @param _digest - The digest of the block
    */
-  function verify(
+  function verifyAttestations(
     Slot _slot,
     Epoch _epochNumber,
     CommitteeAttestations memory _attestations,
@@ -181,8 +184,7 @@ library ValidatorSelectionLib {
       needed: (targetCommitteeSize << 1) / 3 + 1, // targetCommitteeSize * 2 / 3 + 1, but cheaper
       index: 0,
       signaturesRecovered: 0,
-      reconstructedCommittee: new address[](targetCommitteeSize),
-      proposerVerified: false
+      reconstructedCommittee: new address[](targetCommitteeSize)
     });
 
     bytes32 digest = _digest.toEthSignedMessageHash();
@@ -213,10 +215,6 @@ library ValidatorSelectionLib {
 
           ++stack.signaturesRecovered;
           stack.reconstructedCommittee[i] = ECDSA.recover(digest, v, r, s);
-
-          if (i == stack.proposerIndex) {
-            stack.proposerVerified = true;
-          }
         } else {
           address addr;
           assembly {
@@ -230,10 +228,6 @@ library ValidatorSelectionLib {
 
     address proposer = stack.reconstructedCommittee[stack.proposerIndex];
 
-    require(
-      stack.proposerVerified, Errors.ValidatorSelection__InvalidProposer(proposer, address(0))
-    );
-
     require(
       stack.signaturesRecovered >= stack.needed,
       Errors.ValidatorSelection__InsufficientAttestations(stack.needed, stack.signaturesRecovered)

l1-contracts/test/Rollup.t.sol

@@ -864,7 +864,7 @@ contract RollupTest is RollupBase {
         end: _end,
         args: args,
         fees: fees,
-        attestations: CommitteeAttestations({signatureIndices: "", signaturesOrAddresses: ""}), // TODO(palla): Add unit tests with non-empty attestations
+        attestations: CommitteeAttestations({signatureIndices: "", signaturesOrAddresses: ""}),
         blobInputs: _blobInputs,
         proof: ""
       })

yarn-project/ethereum/src/contracts/rollup.ts

@@ -405,7 +405,7 @@ export class RollupContract {
       await this.client.simulateContract({
         address: this.address,
         abi: RollupAbi,
-        functionName: 'validateHeader',
+        functionName: 'validateHeaderWithAttestations',
         args,
         account,
       });

yarn-project/sequencer-client/src/publisher/sequencer-publisher.ts

@@ -344,7 +344,7 @@ export class SequencerPublisher {
     await this.l1TxUtils.simulate(
       {
         to: this.rollupContract.address,
-        data: encodeFunctionData({ abi: RollupAbi, functionName: 'validateHeader', args }),
+        data: encodeFunctionData({ abi: RollupAbi, functionName: 'validateHeaderWithAttestations', args }),
         from: MULTI_CALL_3_ADDRESS,
       },
       {