Our code requires sapling activation at height 1 so we can simplify this function
which should lead to some performance improvements since it is called from many
places, including mining code.
By taking a wallet lock first and then main later we run into
a potential deadlock :
2024-09-13 11:14:37 POTENTIAL DEADLOCK DETECTED
2024-09-13 11:14:37 Previous lock order was:
2024-09-13 11:14:37 (1) cs_wallet wallet/wallet.cpp:985
2024-09-13 11:14:37 (2) cs_main wallet/wallet.cpp:890
2024-09-13 11:14:37 Current lock order is:
2024-09-13 11:14:37 (2) cs_main wallet/wallet.cpp:2845
2024-09-13 11:14:37 (1) cs_wallet wallet/wallet.cpp:2845
Without it we get an AssertLockHeld when calling GetKeyPoolSize .
We could probably make this lock apply to less code, possibly only
the single line that calls GetKeyPoolSize() needs it.
We do not seem to need this lock for the entire DecrementNoteWitnesses function,
we need it only when calling GetSaplingSpendDepth. Also protects against
the case in the future where some code without cs_main calls GetSaplingSpendDepth.
Another possibly better approach would be to take a lock inside
GetSaplingSpendDepth since we want the lock to apply to as little
code as possible and it is GetSaplingSpendDepth that calls GetDepthInMainChain
which requires a cs_main lock .
This allows us to see function names in a backtrace when AssertLockHeld
gives us an assert, which is helping to debug our deadlock bug.
This code should be improved to only do this for debug builds.
This should lower the main thread's likelihood to immediately reacquire
cs_main after dropping it, which should help ThreadNotifyWallets and the
RPC methods to acquire cs_main more quickly.
Ported from ZEC commit e2cd1b761fe556bc6d61849346902c3611530307
