Fix endorsed certificate data race - #8274
Conversation
Publish immutable endorsed certificate snapshots atomically and let pending signature transactions retain the snapshot they were created with. This prevents certificate renewal from racing signature construction and changing an already queued signature. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 0adcbd2f-a656-4fe9-a2fc-2b5f66c3d778
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 0adcbd2f-a656-4fe9-a2fc-2b5f66c3d778
There was a problem hiding this comment.
🟡 Changes recommended
The new TSAN coverage test introduces a tight busy-loop updater that can cause unnecessary CPU/malloc churn and slow or destabilize CI under sanitizers.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR fixes a TSAN-reported data race in the node history signature path by changing endorsed-certificate publication from a mutable in-place object to an immutably-owned snapshot, so pending signature transactions retain stable certificate ownership even if the certificate is concurrently replaced.
Changes:
- Publish
HashedTxHistory’s endorsed certificate viastd::atomic<std::shared_ptr<const Pem>>and load an immutable snapshot when emitting signatures. - Update
MerkleTreeHistoryPendingTxto own the certificate snapshot (shared_ptr) rather than referencing history-owned storage. - Add unit test coverage to deterministically validate snapshot retention across a queued signature, plus concurrent update/emission coverage for TSAN.
Custom instructions used: None (no repository instruction files were explicitly loaded during this review).
File summaries
| File | Description |
|---|---|
| src/node/history.h | Replace mutable optional endorsed cert with atomically-published immutable snapshot; pending signature tx owns snapshot via shared_ptr. |
| src/node/test/history.cpp | Add deterministic regression test for cert snapshot retention and a concurrent updater/emitter loop for TSAN coverage. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Summary
HashedTxHistory's endorsed certificate as an immutable atomic snapshotWhy
The node-endorsed-certificate local hook can call
set_endorsed_certificate()while the periodic signature task is constructing aPrimarySignature. The setter replaced anoptional<Pem>whileMerkleTreeHistoryPendingTxretained a reference to its contents, producing the data race reported by the Long Test TSAN job for #8243.No existing history lock is appropriate here: the setter runs from a KV hook while map locks are held, so taking
state_lockorsignature_lockwould add a new lock-order edge. Atomic publication of an immutable snapshot gives readers stable ownership without extending the hook's lock chain.This is independent of the #8243-#8246 stack. #8209 and the #8238 test harness also touch
history.hand should preserve this snapshot ownership when rebased.Testing
history_testbuild/tests.sh -R '^history_test$'history_testbuild-tsan/tests.sh -R '^history_test$'Long Test CI is enabled on this draft.
Closes #8273