The wallet is an integral part of the bitcoind full node for historic
reasons. See the first git
commit
where the wallet lock cs_wallet is taken after cs_main.
The new interface interfaces::Chain lets the wallet access chain and
mempool state for its own operations. Interface methods allow the wallet to
do things like access the block height to know a transaction’s confirmation
depth or estimate the feerate.
Some heavily-used interfaces::Chain methods impose synchronisation between
wallet and node because Chain::Lock needs to be taken by the wallet
before its own lock.
Switching the lock order from cs_main -> cs_wallet to cs_wallet ->
cs_main would avoid needlessly stalling the node when there is no need to
access chain data.
PR 15931 removed
GetDepthInMainChain’s requirement to lock the chain and simplified the
reasoning on locks and events ordering.
A long-term goal is to separate major components of the Bitcoin Core node (the
wallet, the P2P layer, the GUI) to let them evolve independently and only
communicate asynchronously through well-defined interfaces: See
ryanofsky’s github issue or
eklitzke’s blog post for
reasoning.
To learn more about lock management in Bitcoin Core:
<jonatack> ariard: pulled the PR branch, opened diff locally with gitk, built with --enable-debug, ran tests, ran bitcoind, looked for warnings, read the commits one by on gitk, git grepped varois sites
<jonatack> ariard: after jkczyz commented on the build warning, i then wanted to reproduce, so switched from gcc to clang, which showed the warning for me (reassuring)
<_andrewtoth_> there are comments mentioning this could potentially improve IBD. How would it? Wouldn't it only affect performance if wallet rpcs are called during IBD?
<jonatack> ariard: What gave you the initial idea to begin working on this series of PRs? Something you read, that someone said, or the idea came to you on its own?
<ariard> _andrewtoth_: yes that should change thread concurrency which should be better, I was more thinking about wallet behavior if the sense of different return values or breaking some internal features
<ariard> _andrewtoth_: well actually it may have change if wallet do something like A=getheight(), B=getheight(), if A == B do something, if A != B do something else
<ariard> most of them are related to the rescan code, which currently don't lock the cs_main and verify every previous value returned from the Chain interface
<jonatack> ariard: to check values, as you mentioned verifying values returned. It's interesting to see how others debug bitcoin core. For example, i've learned a lot from watching achow101's live coding sessions on twitch tv
<jonatack> ariard: from watching achow101 (so far): compiler-driven debugging, gdb use, inspecting wallet files using Berkeley DB special commands (located in db4/bin), using flame graphs.
<jonatack> ariard: yes, i tend to begin with prints and asserts but have not found myself deep in the weeds yet in my PRs since they have been simple ones
<ariard> next question: Do you expect this PR to have any impact on performace? But we already talk a bit about this, so let's readjust the question how could we make wallet code better to service concurrently multiple RPC calls ?
<ariard> jkczyz: I think the way of thinking what are the main data structures of the wallet, like coins, keys, address, what can be accessed concurrently and for what we need a synchronized view
<jkczyz> I see what you are saying. I was thinking of different wallets. For the same wallet, should concurrent RPCs that "write" be disallowed then? Or at least while holding a higer-level lock?
<jonatack> (chris stewart has a large PR to add a bunch of PBT that needs review and we ought to add PBT coverage for instance to the BIP157-158 changes, there may be low-hanging fruit there)
<jonatack> afaict #16426 should get in soon if no bugs are found during review, better to merge it not too late in this release cycle to have time to catch anything unexpected
<ariard> okay thanks all for participation, will hang here a bit more if you have questions, and once #16426 merged I will open issues to keep track what can be do as follow-ups
<fanquake> ariard if you want open a follow ups issue in advance of that being merged, feel free. Better to capture all the issues etc now, rather than digging through comments post-merge.