Fix LedgerSecrets lock ordering

[Amaury Chamayou (achamayou)]

Summary

• Take the encrypted-ledger-secrets KV dependency before acquiring LedgerSecrets::lock in get_latest(), get_latest_and_penultimate(), and get().
• Document the required lock order next to dependency tracking.
• Add a focused ledger_secrets_test that runs without TSAN suppressions in TSAN builds.

Why

Local KV map hooks run while their map is still locked. The rekey path calls LedgerSecrets::set_secret() from such a hook, establishing the order:

KV map lock -> LedgerSecrets::lock

The affected read methods previously held LedgerSecrets::lock while take_dependency_on_secrets() opened and read the encrypted-secrets KV map, establishing the inverse order:

LedgerSecrets::lock -> KV map lock

That cycle is an undesirable lock-order inversion and can deadlock when the paths overlap. Taking the KV dependency first means the KV handle operation completes before LedgerSecrets::lock is acquired, so the inverse nested order no longer exists.

This does not weaken synchronization: every access to the in-memory ledger_secrets map remains protected by LedgerSecrets::lock. The KV operation only records the transaction dependency. If a rekey races between dependency tracking and the in-memory read, the transaction dependency preserves conflict detection for committable transactions. The implementation performs the same operations and shortens the mutex critical section, so it introduces no additional allocation, copying, or synchronization overhead.

Regression coverage

The new test calls all three affected read methods, then commits to the encrypted-secrets table with a local map hook that calls set_secret(). This deterministically exposes both lock orders to TSAN without requiring concurrent timing.

For this target only, CTest replaces the repository-wide TSAN suppression setting with detect_deadlocks=1, halt_on_error=1, and second_deadlock_stack=1. A controlled run with the old ordering fails with ThreadSanitizer: lock-order-inversion; the fixed ordering passes.

Testing

• ledger_secrets_test, node_frontend_test, encryptor_test, historical_queries_test, and snapshotter_test in the normal build: all pass.
• ledger_secrets_test in a TSAN build with no suppressions: passes.
• C++ and CMake format checks, include checks, copyright checks, ASCII checks, and git diff --check: pass.

Extracted as a focused fix from the issues investigated in #7744.

[Copilot]

Pull request overview

This PR removes a lock-order inversion between LedgerSecrets::lock and KV map locks by taking the encrypted-ledger-secrets KV dependency before acquiring LedgerSecrets::lock in read paths, and adds a focused TSAN test to catch regressions.

Changes:

• Reorder locking in LedgerSecrets::get_latest(), get_latest_and_penultimate(), and get() to take the KV dependency before acquiring LedgerSecrets::lock.
• Document the required lock order near dependency tracking in LedgerSecrets.
• Add ledger_secrets_test and TSAN-specific test environment settings to detect lock-order inversions without suppressions.

Custom instructions used:

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

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

File	Description
src/node/ledger_secrets.h	Moves KV dependency acquisition before LedgerSecrets::lock and documents lock ordering rationale.
src/node/test/ledger_secrets.cpp	Adds a regression test that exercises both lock orders (read methods + map hook rekey path).
CMakeLists.txt	Registers ledger_secrets_test and configures TSAN options for this test target.

[Amaury Chamayou (achamayou)]

Copilot drop d6c62c1 from the PR please

[Copilot]

Copilot drop d6c62c1 from the PR please

I can't rewrite history/force-push to remove that commit, so I added f9d60a6 which restores the b2e6524 state (the DETECT_DEADLOCKS refactor), undoing d6c62c1's changes. Net effect on CMakeLists.txt/cmake/common.cmake is the same as if d6c62c1 had never been pushed.

[Eddy Ashton (eddyashton)]

Great, the comment explaining DETECT_DEADLOCKS is really what I was looking to be checked in, and having that function will let us do the same for future unit tests as we gradually unpick the suppressions. LGTM!