Fix node certificate synchronization

[Amaury Chamayou (achamayou)]

Summary

• protect mutable node certificates with a dedicated mutex and use stable copies outside its lock scope
• make NodeClient own immutable certificate snapshots instead of references into NodeState
• update recovery-decision-protocol readers and add focused ownership coverage

Issue #8123 coverage

This PR resolves Still Applicable item 3, certificate state lock ordering and ownership:

• certificate KV hooks no longer take the broad NodeState::lock
• both mutable node certificate fields are protected by a dedicated mutex
• certificate lock scopes cover only snapshot publication and never span TLS, history, node-to-node channel, frontend, or RPC work
• asynchronous NodeClient users retain immutable certificate snapshots rather than references that can race renewal

This PR does not resolve items 4-6 or the remaining AFT startup question.

Testing

• Clang 18 Debug builds: node_client_test, ccf, logging
• build-local/tests.sh -R '^node_client_test$'
• focused three-node certificate renewal rollback/re-election partition scenario
• TSAN build and the same focused unit and partition scenario
• rebased onto Fix RpcFrontend consensus and history publication #8127, then rebuilt node_client_test and ccf and reran the focused unit test
• C++/CMake formatting, include, copyright, ASCII, and diff checks

Partially addresses #8123

[Copilot]

Pull request overview

This PR improves node certificate synchronization in NodeState by separating certificate mutation from the broad NodeState::lock, publishing stable certificate snapshots under a dedicated mutex, and ensuring asynchronous internal RPC users (NodeClient) retain immutable certificate copies to avoid races during renewal.

Changes:

• Introduce a dedicated node_certificates_lock in NodeState and use it to protect reads/writes of mutable node certificate state.
• Change NodeClient/HTTPNodeClient to store certificate snapshots by value (immutable) instead of references into NodeState.
• Add a focused unit test (node_client_test) and update recovery-decision-protocol certificate readers to use stable snapshot access.

Custom instructions used:

• .github/copilot-instructions.md
• .github/instructions/reviewing.instructions.md

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated no comments.

Show a summary per file

File	Description
src/node/node_state.h	Adds a dedicated certificate mutex, narrows certificate lock scopes, and passes stable certificate snapshots to TLS/session setup and internal clients.
src/node/node_client.h	Switches certificate ownership from references to immutable by-value snapshots.
src/node/http_node_client.h	Updates constructor/ownership to pass certificate snapshots into the base NodeClient.
src/node/recovery_decision_protocol.cpp	Uses NodeState certificate snapshot accessor and avoids holding broad locks while preparing authenticated messages.
src/node/test/node_client.cpp	Adds a unit test verifying NodeClient retains certificate snapshots after source mutation.
CMakeLists.txt	Registers the new node_client_test unit test target.

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Copilot]

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated no new comments.

[Max (maxtropets)]

LGTM, but I'd say Eddy should have a look, I'm not a lock-master

[Amaury Chamayou (achamayou)]

Eddy Ashton (@eddyashton) can you have a look please?

[Eddy Ashton (eddyashton)]

If LONG_TEST (TSAN) is happy, I'm happy.

[Amaury Chamayou (achamayou)]

If LONG_TEST (TSAN) is happy, I'm happy.

We won't quite have a full happy until the other nodestate lock re-reorderings are complete and we can remove the suppressions (#8144, #8139 and one more that does not exist yet for the frontends).