[grempe]

Fixes #1272.

witnessClient.update read and wrote oldSize without synchronisation. CosignCheckpoint returns cosignatures as they arrive, and the log stops reading them once its policy is met. So when either of two witnesses can meet the policy, the other's update can still be running, or finish unread, when the next checkpoint goes out, and the race detector reports the two updates' accesses to oldSize. An update that finished late could also set oldSize back to an older size.

This gives each witness a mutex, held for the whole update, so a witness's updates run one at a time, and each starts from the size the last one left. If an older update gets the lock after a newer one, its fetchProof fails, since oldSize is then larger than its checkpoint, so it can't set oldSize back. Holding the lock across the request means a slow witness delays its own next update, but only until the earlier update's context ends: the publisher cancels it once that checkpoint is done, or at the witness timeout.

Testing

• New TestWitnessGatewayUnreadUpdates sends nine checkpoints to two witnesses, reads only the first cosignature of each, as a log whose policy either witness meets does, and pauses between them, as a checkpoint interval would. It checks that every update still returns its cosignature.
  • On main, under -race, it fails in 10 of 10 runs, reporting the same read (client.go:337) and write (client.go:372) as the chaos test.
  • With the fix it passes 50 of 50.
• New TestWitnessGatewayLateUpdate holds one witness's update for checkpoint 1 until checkpoint 2 has gone out, then sends checkpoint 3, and checks the old size of each request that witness gets.
  • On main, and with this PR's first version (an atomic.Uint64), it fails in 10 of 10 runs. The late update sets the size back from 2 to 1, so checkpoint 3 goes out with old size 1, not 2.
  • With the fix it passes 50 of 50.
• Race tests: go test -race ./internal/witness/ passes on Go 1.26.10 and 1.27.3.
• Lint: golangci-lint v2.13.2 reports 0 issues.
• Full suite under -race: it passes on macOS apart from TestConstructAndUseAppender (/tmp is not a directory), which fails the same way on main here.

---

Reviewed and submitted by me, a human. The investigation, the fix and its testing were developed with the assistance of an AI coding agent.

[AlCutter]

Hi @grempe, thanks for this (and all the other!) PRs.

This seems better than it was, but I think a per-witness mutex would be better/easier to reason about; e.g. the atomic could potentially be rolled "backwards" to a lower value, locking the update under a mutex should make it clear(er) that this is not possible.

Thoughts?

[grempe]

Thanks! You're right about the atomic, so I've switched to a per-witness mutex held for the whole update: a witness's updates now run one at a time, each starting from the size the last one left. A new test, TestWitnessGatewayLateUpdate, shows the difference. It holds one witness's update for checkpoint 1 until checkpoint 2 has gone out. With the atomic, that late update set the size back from 2 to 1, so checkpoint 3 went out with old size 1; with the mutex it goes out with 2. The lock also means the proof and the request can't disagree on the old size, so I've dropped the single read and left that code as it was. Holding the lock across the request does mean a slow witness delays its own next update, but only until the earlier update's context ends, which the publisher cancels once that checkpoint is done.

[AlCutter]

Thanks @grempe!