feat: Delayed signature verification during block proposals by spalladino · Pull Request #15813 · AztecProtocol/aztec-packages

spalladino · 2025-07-17T21:44:50Z

Implements AztecProtocol/engineering-designs#69

Median propose gas cost is 321250 according to happy.t.sol#test_100_val

LHerskind

Overall think the direction is good, added some comments that are mostly minor or suggestions for future work.

The main important one is the missing invalidation case 👀

LHerskind · 2025-07-18T07:47:15Z

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

+    Epoch epoch = STFLib.getEpochForBlock(_endBlockNumber);
+
+    // Only verify attestations if they are not empty (for testing compatibility)
+    if (!_attestations.isEmpty()) {


Assuming that the if is to be removed and always run later?

Yep, removed, verify already takes care of skipping the check if the committee is empty.

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

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

LHerskind · 2025-07-18T08:26:22Z

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

+    );
+
+    // Get the stored block data
+    TempBlockLog memory blockLog = STFLib.getTempBlockLog(_blockNumber);


You don't need to load the entire thing into memory, a bunch of the values wont be used, you could use TempBlockLog storage blockLog.

Good catch. I added an additional function to STFLib to get the block log as storage directly.

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

LHerskind · 2025-07-18T08:29:22Z

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

+    }
+
+    // Calculate required threshold (2/3 + 1)
+    uint256 requiredSignatures = (committeeSize << 1) / 3 + 1;


Would probably keep the *2 just for readability here 😆 Think we can live with the 2 gas more.

Just copy-pasta from verify :-P

LHerskind

You also want to update the preheatHeaders function in STFLib.sol to not pay a penalty.

LHerskind

Think I found a way to have honest committees and provers but still prune due to the changes in verify 👀.

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

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

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

LHerskind · 2025-07-25T07:31:09Z

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

+    Slot slot = _args.header.slotNumber;
+    Epoch epoch = slot.epochFromSlot();
+    ValidatorSelectionLib.verify(slot, epoch, _attestations, _args.digest);
+    ValidatorSelectionLib.verifyProposer(slot, epoch, _attestations, _signers, _args.digest);


Is the proposer not already validated during the verify in here, so why the separate call to verifyProposer?

Removed the proposer verification from verify above (which I renamed to verifyAttestations)

LHerskind · 2025-07-25T07:36:51Z

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

    require(
-      stack.proposerVerified || proposer == msg.sender,
-      Errors.ValidatorSelection__InvalidProposer(proposer, msg.sender)
+      stack.proposerVerified, Errors.ValidatorSelection__InvalidProposer(proposer, address(0))


This introduce a very strange edge case, or maybe I am misunderstanding again 😬.

Because the verify is applied at the time of proof submission, but the verifyProposer is done at proposer. It is possible to pass 33 attestations that do not include the attester, but have the attester transmit the message. It would pass the verifyProposer check in propose BUT it would fail the verify. At the same time, it cannot be invalidated.

I think this check should actually be removed. We don't care for the validity if the proposer actually attested just that there are enough. We do however care at the time of propose that he controlled it (so that check is good).

Removing this check

LHerskind · 2025-07-25T07:48:51Z

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

+
+    // 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);


You are doing this write twice, first here and then in the _invalidateBlock below.

LHerskind · 2025-07-25T07:58:47Z

l1-contracts/test/Rollup.t.sol

        end: _end,
        args: args,
        fees: fees,
+        attestations: CommitteeAttestations({signatureIndices: "", signaturesOrAddresses: ""}), // TODO(palla): Add unit tests with non-empty attestations


Issue, or part of this?

Just realized I never pushed the commit Add test for submitEpochProof

LHerskind · 2025-07-25T08:16:36Z

l1-contracts/test/validator-selection/ValidatorSelection.t.sol

+    // a block if you submit one with no signatures. This was a change from prior behavior where we had had
+    // that if there were zero validators in a rollup, anyone could build a block
+
+    // TODO(palla): What should we do in this scenario? Block the proposal, or allow invalidating later?


The committee is still the same (why it was revert=true before), so as long as that committee is the active committee you kinda gotta wait. As it could also just be a partial move, you are in this weird state where the chain is degraded.
I believe you will also have a very similar thing going on at the new rollup, because of the delayed reads (2 epochs) the first two epochs after attesters were added there won't be much to do 👀

LHerskind · 2025-07-25T08:18:52Z

l1-contracts/test/validator-selection/ValidatorSelection.t.sol

Not necessarily for this PR, but we gotta clean those up and split it better, can be hard to follow in here. Some of the extra things for invalidation make it a bit harder as well.

Agree, there's also a lot of duplicate code across test helpers

just-mitch

Looks good to me code-wise! We definitely need to update the design to just have the cleaned up flows with their proper names.

just-mitch · 2025-07-27T13:00:50Z

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

+  }
+
  /**
   * @notice  Propose a pending block from the point-of-view of sequencer selection. Will:


Comment should really be updated.

l1-contracts/test/validator-selection/ValidatorSelection.t.sol

just-mitch · 2025-07-27T14:39:21Z

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

validateHeader can be restricted to view now the linter is telling me 🎉

Good point! We no longer sample validators in there

just-mitch · 2025-07-27T14:42:37Z

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

  }

-  function validateHeader(ValidateHeaderArgs calldata _args) external {
+  function validateHeaderWithAttestations(


It would be good to document the overall flows here of who calls what, when. Perhaps the design doc should be updated with the implementation and expected flows.

yarn-project/ethereum/src/deploy_l1_contracts.ts

Implements AztecProtocol/engineering-designs#69

LHerskind

Thanks for addressing it 👍

LHerskind · 2025-07-28T18:30:36Z

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


-  // @note: not view as sampling validators uses tstore
-  function validateHeader(ValidateHeaderArgs memory _args) internal {
+  function validateHeader(ValidateHeaderArgs memory _args) internal view {


Uhh, a view 🙏

Archiver now checks committee attestations and refuses to sync a block if it does not pass validation. Note that this addresses scenarios where the proposer is malicious, but does not handle cases where the entire committee is and produces signatures for a block with an unattested parent. That'll be left for a future PR. Builds on #15813

spalladino force-pushed the palla/delayed-sig-verification branch from 12719d4 to fe70dd3 Compare July 17, 2025 22:04

LHerskind reviewed Jul 18, 2025

View reviewed changes

spalladino force-pushed the palla/delayed-sig-verification branch from 294c5ce to b060765 Compare July 18, 2025 20:06

spalladino marked this pull request as ready for review July 18, 2025 22:13

spalladino requested review from Maddiaa0 and just-mitch as code owners July 18, 2025 22:13

spalladino removed request for Maddiaa0 and just-mitch July 18, 2025 22:13

spalladino force-pushed the palla/delayed-sig-verification branch 3 times, most recently from 5448803 to 4e0a2bc Compare July 22, 2025 01:03

spalladino added the ci-no-fail-fast Sets NO_FAIL_FAST in the CI so the run is not aborted on the first failure label Jul 22, 2025

This was referenced Jul 22, 2025

feat: Archiver does not sync blocks with invalid attestations #15896

Merged

feat: Change sampling algorithm to use feistel permutations #15938

Closed

spalladino force-pushed the palla/delayed-sig-verification branch 2 times, most recently from 621950f to 72f9d52 Compare July 24, 2025 20:10

LHerskind requested changes Jul 25, 2025

View reviewed changes

spalladino force-pushed the palla/delayed-sig-verification branch from 87f2b1f to 2b10c89 Compare July 25, 2025 21:31

just-mitch approved these changes Jul 27, 2025

View reviewed changes

spalladino added 10 commits July 28, 2025 09:25

feat: Delayed signature verification during block proposals

11bc285

Implements AztecProtocol/engineering-designs#69

Address comments from @LHerskind

2030004

Preheating and unit tests

a084485

Fix ts-land

56567c3

Reduce Rollup contract size

c3edca6

Try/catch L1 contract deploy

e183f97

Fix tests

f8612c5

Format

72467ee

Solhint

553058e

Do not sample entire committee when checking proposer

cad8ca4

spalladino added 12 commits July 28, 2025 09:27

Provide signers in calldata during propose

6b9a584

Optimize reconstructCommitteeFromSigners

57d5d44

Add signers to propose in ts-land

c670d46

Enter ExtRollupLib3

5b29e12

Fix bad rebase

ca3894e

Lint

c8066b6

Solhint

6543447

Add test for submitEpochProof

3ed7201

Suggestions from code review

7ad42c0

No msg.sender checks in validator selection

69263f4

Reenable test

9e591cb

Address comments from @just-mitch

c35f762

spalladino force-pushed the palla/delayed-sig-verification branch from 2b10c89 to c35f762 Compare July 28, 2025 12:56

spalladino enabled auto-merge July 28, 2025 12:56

LHerskind approved these changes Jul 28, 2025

View reviewed changes

spalladino added this pull request to the merge queue Jul 28, 2025

Merged via the queue into next with commit cbaa077 Jul 28, 2025
4 checks passed

spalladino deleted the palla/delayed-sig-verification branch July 28, 2025 20:09

Conversation

spalladino commented Jul 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

LHerskind left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LHerskind left a comment

Choose a reason for hiding this comment

Uh oh!

LHerskind left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

just-mitch left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

LHerskind left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

spalladino commented Jul 17, 2025 •

edited

Loading