Skip to content

Fix/attestation last attestation time race - #242

Merged
JamesEjembi merged 6 commits into
VeriNode-Labs:mainfrom
rexx010:fix/attestation-last-attestation-time-race
Aug 28, 2026
Merged

Fix/attestation last attestation time race#242
JamesEjembi merged 6 commits into
VeriNode-Labs:mainfrom
rexx010:fix/attestation-last-attestation-time-race

Conversation

@rexx010

@rexx010 rexx010 commented Aug 28, 2026

Copy link
Copy Markdown

Closes #213

Problem

The attestation batch processor uses a 4-worker pool to process attestations concurrently. When multiple attestations for the same node land in the same batch, workers process them in parallel and each writes nodes.last_attestation_time. Because writes commit in claim order, not timestamp order, a worker holding an older attestation timestamp (T1) can commit after a worker holding a newer one (T3) — leaving the column at T1 even though T3 is more recent.

Downstream, livenessChecker reads last_attestation_time to flag nodes as stale after 48h of silence. A clobbered timestamp can trigger a false "stale node" alert even though the node attested recently.

This repo didn't yet have the attestation pipeline this bug was reported against (no nodes/attestations tables, no batch processor, no liveness checker), so this PR builds the pipeline and applies the fix in the same pass, following the atomic-update convention already used in src/reputation/store.ts.

Fix

AttestationStore.advanceLastAttestationTime() (src/attestation/store.ts:55-71) updates the column with:

UPDATE nodes
SET last_attestation_time = GREATEST(last_attestation_time, $2)
WHERE id = $1

GREATEST() ignores NULL arguments (only returns NULL if every argument is NULL), so this is also correct on a node's very first attestation. The column can now only move forward, regardless of which worker's transaction commits last.

What's included

File Lines Purpose
src/database/migrations/014_attestation_pipeline.sql 26 New nodes + attestations tables, partial index on pending rows
src/attestation/store.ts 89 assignWork() (FOR UPDATE SKIP LOCKED claim query) + the GREATEST() fix
src/attestation/worker.ts 57 processOne() — applies the reputation reward and the monotonic timestamp advance in one transaction
src/attestation/batchProcessor.ts 63 AttestationBatchProcessor — 4-worker pool draining up to 100 attestations/batch
src/monitoring/livenessChecker.ts 40 findStaleNodes() / isStale() — 48h liveness check
tests/attestation/batchProcessor.test.ts 212 Test coverage (see below)

Tests

Two tests in tests/attestation/batchProcessor.test.ts:

  1. testOutOfOrderWritesStayMonotonic — directly reproduces the bug scenario: writes commit in the order T3, T2, T1. Asserts the column ends at T3 (the max), not T1 (the last write in a naive implementation).
  2. testFourWorkersTenAttestationsSameNode — runs the real AttestationBatchProcessor with 4 workers over 10 attestations for one node with shuffled timestamps. Asserts last_attestation_time == MAX(all timestamps), every attestation is processed exactly once, and the reputation reward is applied exactly 10 times.

Run directly: npx tsx tests/attestation/batchProcessor.test.ts

Verified locally:

  • npx tsc --noEmit — 0 errors, project-wide
  • npm test — full suite green, including the new file

Notes for reviewers

  • Considered the other options in the issue's resolution blueprint (batch-level max update, explicit node-row locking, a versioned last_attestation_seq counter) but GREATEST() alone satisfies the monotonicity invariant with the least contention, and matches the pattern already established in src/reputation/store.ts.
  • Tests were built and verified against an in-memory fake Database/PoolClient (no live Postgres in the dev environment used to prepare this PR) — a real-DB smoke test before merge is worth doing given that.

Dev added 6 commits August 28, 2026 07:55
… pipeline

Adds the tables the batch processor needs: 'nodes' (holding the
monotonic last_attestation_time watermark) and 'attestations' (the
work queue that workers claim from). Partial index on pending rows
keeps FOR UPDATE SKIP LOCKED scans cheap as the queue grows.

Migration: src/database/migrations/014_attestation_pipeline.sql
…EATEST()

Root cause: concurrent workers processing attestations for the same
node write last_attestation_time in claim order, not timestamp order.
A worker holding an older timestamp (T1) that commits after a worker
holding a newer one (T3) overwrites the column back to T1, which can
trigger a false 'stale node' liveness alert.

Fix: AttestationStore.advanceLastAttestationTime() now runs
  UPDATE nodes SET last_attestation_time = GREATEST(last_attestation_time, $2)
  WHERE id = $1
so the column can only move forward regardless of write order.
Postgres's GREATEST() ignores NULL args, so this is also safe on a
node's first-ever attestation.

Also adds assignWork() (SELECT ... FOR UPDATE SKIP LOCKED, the work-
claim query workers use) and markProcessed()/ensureNode() helpers.

File: src/attestation/store.ts (new, 89 lines)
Key change: lines 55-71 (advanceLastAttestationTime, the GREATEST update)
Applies the +10 reputation reward (via the existing ReputationStore.
applyRewardWithLock, reused as-is) and the monotonic timestamp advance
from the same transaction the caller claimed the row in, then marks
the attestation processed.

File: src/attestation/worker.ts (new, 57 lines)
Runs a pool of workerCount (default 4) concurrent workers, each
claiming one attestation at a time via assignWork() (FOR UPDATE SKIP
LOCKED) inside its own transaction, until maxBatchSize (default 100)
attestations are drained or the pending queue is empty.

File: src/attestation/batchProcessor.ts (new, 63 lines)
findStaleNodes() flags nodes whose last_attestation_time is NULL or
older than the 48h threshold. Depends on the monotonicity fix in
AttestationStore, since a clobbered timestamp here is exactly what
produces a false stale-node alert.

File: src/monitoring/livenessChecker.ts (new, 40 lines)
Two tests (resolution item 5):
1. testOutOfOrderWritesStayMonotonic - directly reproduces the issue's
   exact scenario (writes commit T3, T2, T1 in that order) and asserts
   the column ends at T3, not T1.
2. testFourWorkersTenAttestationsSameNode - runs the real
   AttestationBatchProcessor with 4 workers over 10 attestations for
   one node (shuffled timestamps), asserting last_attestation_time ==
   MAX(all timestamps), each attestation processed exactly once, and
   the reward applied exactly 10 times.

Run: npx tsx tests/attestation/batchProcessor.test.ts (both pass)

File: tests/attestation/batchProcessor.test.ts (new, 212 lines)
@JamesEjembi
JamesEjembi merged commit 51b1175 into VeriNode-Labs:main Aug 28, 2026
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Attestation Batch Processing Order Violation in Multi-Core Worker Pool

2 participants