Technically we should take this lock but it has never been there before
and it leads to potentially large slow downs when mining with multiple cores.
We see basically the same hashrate for a single core if we have the lock or
not and that makes sense, since there is only one core, there are no other
mining threads that have to wait. But on one particular CPU I saw a 6% slower
hashing when mining with 2 threads and 35% slower with 3 threads.
This change also means debug builds will coredump if mining is enabled.
Technically we should take this lock but it has never been there before
and it leads to potentially large slow downs when mining with multiple cores.
We see basically the same hashrate for a single core if we have the lock or
not and that makes sense, since there is only one core, there are no other
mining threads that have to wait. But on one particular CPU I saw a 6% slower
hashing when mining with 2 threads and 35% slower with 3 threads.
This change also means debug builds will coredump if mining is enabled.
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.
