The PR branch HEAD was 1125fd92b at the time of this review club meeting.
Notes
This PR is one change in a sequence of PRs making the bitcoin wallet code more
asynchronous and independent of node code, so the node can be more robust and
performant, and not get locked up while wallet code is executing. The changes
are also supposed to be architecture improvements, making the wallet/node
interface more high-level and less tied to implementation details of the node
and wallet. They also get wallet code closer to being able to work offline or in
a different process without constantly having to query the node for information.
Notable PRs making wallet code more asynchronous are:
#10286 by TheBlueMatt –
changing the wallet to process BlockConnected, BlockDisconnected, and other
notifications from the node asynchronously, adding
CWallet::m_last_block_processed field
#15931 by ariard – changing
the wallet to cache block heights, adding
CWallet::m_last_block_processed_height and CWalletTx::m_confirm.block_height
fields, and using cached heights to avoid needing to call
Chain::Lock::getBlockDepth() to determine transaction status.
#17443 by ariard, pending
review – using cached block heights to avoid needing to call Chain::checkFinalTx
to determine transaction status
#17954 by ryanofsky, this pr
– using cached block heights and CWallet::m_last_block_processed in various
wallet functions to avoid the need to call Chain::Lock, and to make the wallet
return internally-consistent information when m_last_block_processed lags
behind the current node tip
#16426 by ariard, pending
review – removing the interfaces::Chain::Lock class altogether to remove a way
wallets could lock up the node unintentionally or longer than needed
All interactions between the wallet and node code happen though the
interfaces::Chain
and
interfaces::Chain::Lock
interfaces. Generally the less the wallet needs to call these interface methods,
the more efficient both the node and the wallet can be. Also the simpler wallet
code can be because it can avoid the headache of having to reconcile its cached
state with latest information returned from the node, in cases where
CWallet::m_last_block_processed has fallen behind the current node tip.
What happens in the interfaces::Chain::Lock chain constructor and
destructor? What is the effect of writing auto locked_chain =
pwallet->chain().lock();?
What’s the difference between calling interfaces::Chain::getHeight() and
CWallet::GetLastBlockHeight()? When will the values be the same, and when will
they be different?
In commit “wallet: Avoid use of Chain::Lock in
listsinceblock”
the call to locked_chain->getHeight() is replaced with
pwallet->GetLastBlockHeight(). If listsinceblock
is called with a non-null block hash, how does this change prevent transactions
that are older than the specified block from being mistakenly printed?
Trickier: In the same commit, the call to
locked_chain->getBlockHash(last_height) is replaced with
pwallet->GetLastBlockHash(). How does this change result in a more accurate
lastblock value that won’t result in missing transactions when passed to a
subsequent listsinceblock call?
In the same commit, what is the coinbaseKey.MakeNewKey(true) line doing,
and why does the test fail if it is removed?
<ryanofsky> Reminder that everyone is here to learn, and all types of questions are encouraged. Anything that seems confusing is probably confusing to other people, and good to spend time figuring out together
<ryanofsky> I can say that the motivation for this is to be able to get rid of the Chain::Lock interface. Because that interface is low level and lets wallets slow down and potentially lock the node
<rockzombie2> My limited understanding (not specifically related to bitcoin): Sharing resources among threads can cause confilcts in state. One solution is to "lock" a resource. There are many ways to be "thread-safe". Some more topics for further research that might help in understanding: Dining Philosophers problem, Spin-locks, Software Transactional Memory.
<ryanofsky> yes, this gets to the question: What’s the difference between calling interfaces::Chain::getHeight() and CWallet::GetLastBlockHeight()? When will the values be the same, and when will they be different?
<ryanofsky> the next question looks into a specific example: In commit “wallet: Avoid use of Chain::Lock in listsinceblock” the call to locked_chain->getHeight() is replaced with pwallet->GetLastBlockHeight(). If listsinceblock is called with a non-null block hash, how does this change prevent transactions that are older than the specified block from being mistakenly printed?
<rockzombie2> Sorry, I'm still very new to this review process, but I'm hoping I can ramp up quickly by attending these reviews because I want to be helpful. As for testing your behavior changes, what are the cases I should try to test (and how exactly can I cover that test case in a multi-threaded environment?)
<ryanofsky> rockzombie2: great question, anybody have any suggestions for testing? i will say bitcoin core doesn't currently have great for testing infrastrucuture to force code to run in order and simulate races
<nehan_> ryanofsky: yes. and in the new behavior, in the case where the wallet is behind chain, it will print fewer transactions. but it's not clear to me why those "weren't requested"
<ryanofsky> in order for there to be a race condition, the "auto locked_chain = pwallet->chain().lock();" line above has to be removed, which is removed in a later PR
<ryanofsky> but hopefully it at least suggests what the problem is, because this example is simpler than other cases like the one ariard was talking about that involves separate RPC calls
<jonatack> rockzombie2: WRT what cases to try to test, one thing you can try is look at the cases ryanofsky describes in the commit message, then look at the relevant unit and functional tests to see if the case is tested, and how (and if not, try adding it)
<jonatack> for instance, in commit e276b682, look at the tests that are added in that commit, and at the those in test/functional/wallet_listsinceblock.py
<nehan_> how important is it to preserve all the little behavioral quirks in the wallet as you make these changes to pull out cs_main? do you know who relies on listsinceblock?
<ryanofsky> the changes within listsinceblock are just meant to be minimal, if it would be simpler to change behavior in some way, that's be reasonable to do
<ryanofsky> maybe someone wants to reason through the next question about listsinceblock: In the same commit, the call to locked_chain->getBlockHash(last_height) is replaced with pwallet->GetLastBlockHash(). How does this change result in a more accurate lastblock value that won’t result in missing transactions when passed to a subsequent listsinceblock call?
<ariard_> rockzombie2: you should test this on the cli, by doing reorg and calling RPC in a while loop, at least I remember doing this while working on some refactoring
<ryanofsky> ariard_, yes i think you're right, there can only be missing transactions if there's a reorg. the other case would result in hitting an assert
<ryanofsky> IMO, code is easier to reason about if you only have to think about the wallet last block processed, and return information relative to that
<jonatack> ryanofsky: when you wrote "In the same commit, the call to locked_chain->getBlockHash(last_height) is replaced with pwallet->GetLastBlockHash()"...
<rockzombie2> I have a general question about the github Bitcoin group. Who exactly are all of those contributors? Like, how did they qualify and become the main body that accepts PRs to bitcoin core?
<ryanofsky> jonattack: ok hopefully clearer now, but let me know if it's not. there is one new line replacing two deleted lines there, and I was asking about that change