fix: preserve signing history across rollup upgrades by alexghr · Pull Request #352 · aztec-labs-eng/aztec-node

alexghr · 2026-09-28T11:40:41Z

The node no longer deletes signing history for other rollups, instead which signing duty records an 'expiry time' and jobs get deleted once they're stale. Existing jobs in the db will get 30d grace period, new jobs get the slashing window as a lifetime.

Fix A-2184

greptile-apps · 2026-09-28T11:45:41Z

Confidence Score: 4/5

[Critical risk] Changes signing history retention across validator upgrades.

The PR should not merge until existing signing records receive the stated migration grace period.

Findings

Existing duties lose grace period ▶
Cleanup lacks matching index ▶
Rollup cleanup isolation lacks coverage ▶

Summary

The PR stops deleting other rollups’ signing history on startup, scopes completed-duty cleanup to the serving rollup, and derives default retention from on-chain slashing settings.

The promised grace period for existing records is not implemented.
The new cleanup predicate lacks a matching index and direct cross-rollup cleanup coverage.

Diagram

%%{init: {'theme': 'neutral'}}%%
flowchart LR
  A[Node starts] --> B[Read slashing settings]
  B --> C[Set default retention]
  C --> D[Periodic cleanup]
  D --> E[Delete completed duties for this rollup older than retention]
  D --> F[Preserve duties for other rollups]

_{Reviews (1) · Last reviewed commit: f08aacd}

greptile-apps · 2026-09-28T11:45:44Z

+          config.cleanupOldDutiesAfterHours = Math.ceil(
+            (Number((offset + lifetime + 1n) * roundSize) * Number(slotDuration)) / 3600,
+          );


Existing duties lose grace period

When an upgraded validator starts with the default retention setting, its first cleanup applies the new slashing-window age to records already in the database. Signed duties older than that window are deleted immediately, rather than receiving the promised 30-day grace period. The cleanup needs to distinguish pre-upgrade records before applying the new limit.

Knowledge Base Used: Validator services

this is addressed through the migration.

You're right — migration 3 adds expires_at with a CURRENT_TIMESTAMP + INTERVAL '30 days' default, so existing rows receive the intended grace period before expiry-based cleanup runs. This addresses my concern.

greptile-apps · 2026-09-28T11:45:44Z

+  AND rollup_address = $2
+  AND completed_at < CURRENT_TIMESTAMP - ($1 || ' milliseconds')::INTERVAL;


Cleanup lacks matching index

The cleanup query now filters by rollup address and completed_at, but the existing cleanup index covers status and started_at. As signing history grows in a shared database, each replica's periodic cleanup may have to examine many unrelated signed rows. An index matching the new filter would keep this maintenance work efficient.

Knowledge Base Used: Operations and deployment

greptile-apps · 2026-09-28T11:45:44Z

      // Clean up duties older than 1 hour
      const maxAgeMs = 60 * 60 * 1000; // 1 hour
-      const numCleaned = await spDb.cleanupOldDuties(maxAgeMs);
+      const numCleaned = await spDb.cleanupOldDuties(ROLLUP_ADDRESS, maxAgeMs);


Rollup cleanup isolation lacks coverage

This test cleans up records for only one rollup. The new cross-rollup startup test checks that records survive startup, but not that aged records for another rollup survive cleanup. Add two-rollup cleanup cases for PostgreSQL and LMDB so a regression in the central isolation guarantee is caught.

Knowledge Base Used: Validator services

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

spalladino

Why do we need a new column for this? Can't we just use started_at plus the max age?

Also, why does the expires_at use the slashing window time? Strictly speaking, we only need to hold the row in place for the duration of a slot, which is when double-signing could occur. After that, we don't really care. So holding a row for a few minutes is good enough, anything in the order of days is plenty.

spalladino · 2026-09-29T14:51:32Z

 DELETE FROM validator_duties
-WHERE status = 'signed'
-  AND started_at < CURRENT_TIMESTAMP - ($1 || ' milliseconds')::INTERVAL;
+WHERE expires_at <= CURRENT_TIMESTAMP;


This cleanup no longer filters on status = 'signed', and lmdb.ts:278 is the same. So it also deletes rows still in signing, which are live locks.

DELETE FROM validator_duties WHERE expires_at <= CURRENT_TIMESTAMP;

You can reach this with VALIDATOR_HA_OLD_DUTIES_MAX_AGE_H=0. The config accepts it (parseSafeInteger, zod .min(0) in stdlib/src/ha-signing/config.ts:87), so the row is inserted with expires_at = now. Any replica's cleanup then removes it while the owner is still signing, and a peer polling the row inserts a fresh one and signs. The owner's recordSuccess fails and it doesn't broadcast, so this is a lost duty rather than a double signature. Cleanup should still never remove a lock.

Reject 0 in config (.positive() in zod plus a > 0 check on the env value) and always default to a positive retention.

written by claude

spalladino · 2026-09-29T14:51:33Z

+    );

    describe('cleanupOldDuties', () => {
      it('should only clean up old signed duties', async () => {


"should only clean up old signed duties" still asserts the signing row survives, but it now survives for a different reason. The shared test config (lines 54-60) has no cleanupOldDutiesAfterHours, so the row is inserted with a NULL expires_at.

postgres.test.ts (~line 1500) has the same problem: the signing fixtures omit expires_at and inherit the +30 days column default. The cleanupOldDutiesAfterHours: 0.5 and 0.000001 settings on the cleanup services no longer affect what cleanup deletes.

So no test covers a signing row whose deadline has passed, which is the case in the schema.ts cleanup. Add a case with an expired signing row on both backends, so a test pins down what cleanup does with a live lock.

written by claude

alexghr · 2026-10-01T13:04:30Z

new column so that we can avoid deleting rows that are in the db at migration time. Otherwise we'd delete everything of the old rollup.

There's no reason to choosing the slashing period other than it's jsut a number 🙂

spalladino · 2026-10-09T13:44:49Z

new column so that we can avoid deleting rows that are in the db at migration time. Otherwise we'd delete everything of the old rollup.

My understanding is we'd delete everything that's expired, right? Why is this a problem? I may be missing something though.

alexghr · 2026-10-09T15:14:49Z

if there are two versions of the node (v5 and v6) both sharing a database:

if encode expiry as start_time + N seconds. The N could different between node versions so they'd thrash each other's cleanup
if don't do anything and leave everything as is: as soon as v6 comes online it deletes all of v5 duties. If v5 restarts it deletes v6 duties (see code snippet below)
if we only delete by rollup address then v6 never cleans up after v5.

If we add a column which says "it's safe to delete this row no sooner than this " than all future versions could encode the same rule.

https://github.com/aztec-labs-eng/aztec-node/pull/352/changes#diff-76d2888ae2b022296211044bae952e3020ba13d4d572a695fa8cd1484b7d7013L230-L239

spalladino · 2026-10-09T15:45:58Z

Hmm I still push back (sorry for being annoying on a low severity issue, but hey, sometimes it's fun to have real discussions with a real dev about real code rather than via agents!). We are setting N ridiculously large, whereas it's only needed for a handful of slots. Even if slot time changes a lot between versions, we should be covered.

alexghr · 2026-10-09T17:20:21Z

sure, I can reduce the number. I just needed a value. It felt natural to have it around for a slashing round

spalladino · 2026-10-09T17:29:47Z

Sorry, my comment was not about reducing the number, but that since it's ridiculously large, there's no need to have the expires-at column, since any deviations in the actual time we need to keep the signature (which is in the order of a few minutes) across node versions won't really matter. So we can drop the new column altogether.

alexghr requested a review from spalladino as a code owner September 28, 2026 11:40

alexghr force-pushed the ag/work-a2184 branch from f08aacd to 6e8ad07 Compare September 28, 2026 11:41

greptile-apps Bot reviewed Sep 28, 2026

View reviewed changes

fix: preserve signing history across rollup upgrades

fc3bca6

alexghr force-pushed the ag/work-a2184 branch from 6e8ad07 to fc3bca6 Compare September 28, 2026 12:10

spalladino reviewed Sep 29, 2026

View reviewed changes

		AND rollup_address = $2
		AND completed_at < CURRENT_TIMESTAMP - ($1 \|\| ' milliseconds')::INTERVAL;

Conversation

alexghr commented Sep 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

greptile-apps Bot commented Sep 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Confidence Score: 4/5

Findings

Summary

Uh oh!

greptile-apps Bot Sep 28, 2026

Choose a reason for hiding this comment

Uh oh!

alexghr Sep 28, 2026

Choose a reason for hiding this comment

Uh oh!

greptile-apps Bot Sep 28, 2026

Choose a reason for hiding this comment

Uh oh!

greptile-apps Bot Sep 28, 2026

Choose a reason for hiding this comment

Uh oh!

greptile-apps Bot Sep 28, 2026

Choose a reason for hiding this comment

Uh oh!

spalladino left a comment

Choose a reason for hiding this comment

Uh oh!

spalladino Sep 29, 2026

Choose a reason for hiding this comment

Uh oh!

spalladino Sep 29, 2026

Choose a reason for hiding this comment

Uh oh!

alexghr commented Oct 1, 2026

Uh oh!

spalladino commented Oct 9, 2026

Uh oh!

alexghr commented Oct 9, 2026

Uh oh!

spalladino commented Oct 9, 2026

Uh oh!

alexghr commented Oct 9, 2026

Uh oh!

spalladino commented Oct 9, 2026

Uh oh!

This branch has not been deployed

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

alexghr commented Sep 28, 2026 •

edited

Loading

greptile-apps Bot commented Sep 28, 2026 •

edited

Loading